[PATCH] ALPN validation fix

Achim Gratz Stromeko at nexgo.de
Sun Dec 8 14:10:21 UTC 2019


Hal Murray via devel writes:
> Thanks.  Interesting that you are the first to notice.  It's been there since 
> mid September.

It doesn't always happen and then not with all NTS servers.  But the
spec is pretty clear that you must not expect a NUL character at the end
of the string.

>> so you can't use strcmp to check
>        memcpy(buff, data, len);
>        buff[len] = '\0';
> The idea was to turn it into a string so it could be printed and we could use 
> string routines.

You don't need to in most cases if you use the correct library routines.

> The bug in the old code was that when this area was reworked back in 
> September, I missed changing the compare to use the new copy.
> -	if (0 != strcmp((const char*)data, "ntske/1")) {
> +	if (0 != strcmp(buff, "ntske/1")) {
> So it would work if the next byte in data was a 0 which seemed to happen on 
> many of my systems.  (Interesting how long it took me to figure that out.)

See above.  We shouldn't copy data around just to compare the ALPN
string.  What we do in the error case can be much  more involved, but
the common case should avoid duplication.

> +		strlcpy(buff, (const char *)data, sizeof(buff));
> That can run off the end of data.

True.  The length argument must instead be the minimum of len and
sizeof(buff):

+		strlcpy(buff, (const char *)data,
+			(len < sizeof(buff)) ? len : sizeof(buff));

> I think there are two approaches.  One is to convert data to a string, then 
> use string routines.  The other is to use memcmp, then convert to a string if 
> you want to print it.  The latter seemed cleaner to me since there is only one 
> place where it gets printed.

No, that's why strncmp exists and we do in fact know that one of the
strings is a proper C string.  The whole business of sanitizing the ALPN
should also go away later on.  If we think we might need to sanitize
strings, then that should be done someplace else or in a libary routine,
but not open-coded.



Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada



More information about the devel mailing list