Re: iscsid - lfs and ipv6 issues

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Fri, 17 Nov 2023 22:22:24 -0000 (UTC)
    From:        mlelstv%serpens.de@localhost (Michael van Elst)
    Message-ID:  <uj8p30$57t$1%serpens.de@localhost>

  | The address parser looks broken.

It certainly is, it is horrid.

  | For some reason the first character is skipped when it tries
  | to identify IPv6,

At the relevant point it doesn't really seem to care which addr
family, but is trying to deal with v6 address literals

  | I was successful with
  | iscsictl add_send_target -a 'x[ipv6-address]'

I can't imagine how that would work (how it avoids the current problem
is clear) - the relevant function simply copies the address (as a string)
to be processed later - for current purposes I didn't look to see how
that is processed into an actual sockaddr type address, and how that
can possibly work with that 'x' there, but if it does, there is very likely
more dubious code.

The actual arg parsing of that -a option (for add_send_target, maybe
other commands as well) is in

	src/sbin/iscsictl/iscsic_parse.c

and the relevant function is get_address()

After checking that the address is present (not NULL or "") the code
does ...

        /* is there a port? don't check inside square brackets (IPv6 addr) */
        for (sp = str + 1, val = 0; *sp && (*sp != ':' || val); sp++) {
                if (*sp == '[')
                        val = 1;
                else if (*sp == ']')
                        val = 0;
        }

That "sp = str + 1" is your "skips the first character" - the problem is
that if the '[' is the first char (which you'd normally expect it to be,
if present at all) then it will never be seen, so val will remain 0, the
first ':' that is seen will then appear to be a ':' that separates the
address from the port number (rather than just part of the syntax of the
v6 address) and it is all downhill after that.   (When you put that 'x'
there, the '[' is seen, and everything up to the ']' is just treated as
the v6 addr, so this part of the code works.)

Simply removing that ' + 1' from the init of sp should fix that one.

But that's just the beginning of the problems...

The code goes on:

        if (*sp) {

If that's true, we know that *sp == ':' from the loop above, but there
are two cases possible, one is an addr, followed by ':' and a port number.
The other is a literal IPv6 addr which isn't enclosed in [ ] (in which case
no port number would be possible, but that's the user's choice).

The code needs to work out which case we have, and it does that by:

                for (sp2 = sp + 1; *sp2 && *sp2 != ':'; sp2++);

That is, simply look at the string following the ':' and see if there's
another ':' later, if there is, then the assumption is that this is
a v6 addr which didn't include the [ ] as protection.   That's OK (though
there are a million ways this stuff can fail to correctly handle various
broken input formats).

                if (!*sp2) {

That is, there are no more ':' in the address, so the code assumes that
what follows the first one (*sp) is a port number, and parses that.

                        /* truncate source, that's the address */
                        *sp++ = '\0';

Now sp points past the ':' (which has been obliterated) at the port
number itself, and the string pointed to be str is the address, from which
the port number (and anything which follows) has been removed.

This is followed by code that parses the value of the port number,
and while it isn't particularly resilient against errors [aside: scanf
makes for crappy parsers, use strtol() instead], it isn't relevant to
anything here, so I'll skip that.

		}

After this, the code wants to move past the just parsed port number,
and see if that was terminated by '\0' or ',' - in the latter case
a group tag (whatever that is, and no, I don't need to be told) can
follow.

		for (; isdigit((unsigned char)*sp); sp++);

That's skipping the digits that were the port number - but note that is
being done, whether or not there was a port number given (oops) - in
the case above where *sp2 == ':' (that is, we have a v6 addr with no
[ ] around it) then this is just nonsense (this is what's happening in
the case that was reported).

                if (*sp && *sp != ',')
                        arg_error(arg, "Bad address format: Extra character(s) '

When that loop is done, we either stopped on the ',' or the '\0' (or
that's what was expected) - if it stopped on anything else, that's
an error.   In the reported case it is stopping at a later ':' in the
v6 addr, which implies that after the first ':' before the second, was
a string of entirely decimal digits (or nothing in a case like fe80::...)
and not one of the alpha hex chars that can make up an IPv6 addr, eg
mine is currently:
	2001:fb1:12a:....
in that case it would complain that the 'f' is an invalid "extra character".

Next the code goes on to the case where there was no port number (no ':'
was seen outside [ ] in the address) ..

        } else
                for (sp = str + 1; *sp && *sp != ','; sp++);

that's skipping everything in the addr, up to a ',' (neither v6 nor v4
addresses contain commas, so that's superficially OK, if brittle)

Then then if the ',' was found in either of the two cases above

        if (*sp) {

If there's no error, when this is true, *sp must be ','

Then there's code that parses the group tag (again not very
reliably, but if it is valid, it should work, so isn't relevant
here)  It ends with:

	                *sp = '\0';

which removes the ',' and all that came after it, to so that what
came before in the case that no port number was present, can be
treated as the address - if there was a port number a '\0' was
already inserted into the string, so this one is not needed - but
is harmless.

        }

After that, whatever is pointed to by str must be the address (the
port number and group tag have been removed from its suffix) and the
remaining code simply makes sure that string isn't too long, and then
copies it to be processed elsewhere (that's where I didn't look, and
where I'd have expected that leading workaround 'x' - which still remains -
would have caused problems - it certainly should if that code is good).

I don't have anything with which to test this, so I am not going to
attempt to make any changes, but what I suggest is to get rid of
that "+ 1" as noted above, move the loop which skips past the port
number inside the "if" test true case where the port number is
processed (it makes no sense at all to skip the thing if it isn't
present) - either that, or change that loop from

	for (; isdigit((unsigned char)*sp); sp++);
into
	for (; *sp && *sp != ','; sp++);

instead, and simply skip anything that is present up to the end, or
the group tag follows indicator (',').

In the first case (moving the isdigit() loop where it belongs) then
the error test code would need to be changed from

	if (*sp && *sp != ',')
to
	if (!*sp2 && *sp && *sp != ',')

so that the case where a v6 addr which has no [] isn't treated as bad.
ALternatively also move that error checking code inside the 'if (!*sp2)'
true case (same effect).

In the second case (changing the loop which advances sp from isdigit()
to the ',' check, then the error test simply needs to be removed, it
would make no sense at all.    I'm not sure why it was considered necessary
to check for that particular aspect of bad syntax, when so many others
are let through unchecked, so just deleting that one isn't going to do
any real harm.

kre