[gnutls-devel] gnutls_prf not compliant to RFC 5705 (or confusingly so)

[Nikos Mavrogiannopoulos]

On Sun, Jul 19, 2015 at 8:47 PM, Rick van Rein <rick at openfortress.nl> wrote:
> Hello,
> The gnutls_prf() function takes arguments “extra” and “extra_size”,
> which are meant to conform to what RFC 5705 calls a “context”.
>  1.
>
>     The function docs state that "extra" is optional, but passing NULL
>     in it causes a memcpy() with NULL source, which causes SEGFAULT (on
>     Linux) even when copying 0 bytes.

That's quite strange. I don't seem to be able to reproduce but it
could be libc-version specific.

>  2.
>     The RFC states that the context is preceded with its length in 16
>     bits, if a context is provided. But gnutls_prf() does not check the
>     length to fit in 16 bits, and it does not prefix the length. The
>     function documentation also does not state that the caller must
>     prefix the length -- which is probably not the idea anyway.

That's quite unfortunate. It seems that gnutls_prf() was not made to
follow rfc5705 but rather an earlier draft and was not updated in the
final version. I'm not sure whether gnutls_prf() should be updated to
fix that issue. One reason is that there may be users of gnutls_prf()
which already prefix extra properly to get the expected output, and
the other is that gnutls_prf() is quite complicated. It has this
"server_random_first" option, which while it may have been useful in
pre-rfc5705 times, now it is plainly confusing.

Hence, I think it would be cleaner to introduce another API to follow RFC5705.

>     The different naming of “extra” from “context” was confusing me when
>     I was trying to use the function from the API spec plus the RFC;
>     perhaps the RFC-name “context” is a good name to preserve.
> If you agree that these are potential places for improvement, then
> please find patches to resolve the above attached.

Thanks for bringing that up. I'll document that gnutls_prf() does not
provide the expected for RFC5705, and add gnutls_prf_rfc5705() which
will be easier to use. I'd appreciate if you could check the new
function; it is uses gnutls_prf(), so you could use something similar
directly.
https://gitlab.com/gnutls/gnutls/commit/e4cddac1b97352587e8eefaf5c9e1f8dfcf2ad53

>   * The 3.2.21 patch was made against the version in which I encountered
>     the problem, and against which I patched it.
>   * The 3.3.16 patch is virtually the same, but against stable head; it
>     has only been tested to compile well.
>   * Note that I am not aware of an easy way to compare the generated PRF
>     with a known-correct value.

It is also unfortunate that the RFC doesn't include any known values.
I've included a known answer test in tests/prf.c in the repository.

regards,
Nikos