Work around GNU TLS hard deprecation of MTI hash functions

[zdohnal]

Hi,

I did further cryptographic review of packages which we ship in our operating systems regarding security certifications - e.g. FIPS - and I found out that there are several usages of MD5 algorithm, which is not considered to be safe according to FIPS certification and current security standards. Additionally Fedora and Red Hat Enterprise Linux are moving towards the ability to set the crypto policy on system-wide level [https://fedoraproject.org/wiki/Changes/CryptoPolicy] and MD5 is not allowed in the default settings/profile.

I plan to create a patch for replacing MD5 with a more secure algorithms for cryptographic use cases plus relaxing gnutls for non cryptographic use cases e.g. generating UUID and cookie and I would like to get it to the upstream sources, but I am not completely sure whether this is something that the project could be interested in and potentially accept? Additionaly if you have any guidance or advices to make the patch more acceptable for the project, please let me know.

Using secure and more recent crypto algorithms is beneficial not only for security certifications, but also in general, therefore I hope that you will consider changes that I would like to propose.

Thank you for your time and have a nice day,

Zdenek

[michaelrsweet]

@zdohnal What uses of MD5 are you concerned about in CUPS?

AFAIK, none of the remaining uses of MD5 matter:

• MD5 for Digest authentication is a requirement unless the server/printer supports SHA2 (which we detect and use automatically)
• MD5 for UUID generation is OK because there is no way to do prefix attacks and we don't use a lot of bits anyways
• MD5 for showing a digest of a certificate chain to the user is purely informational - the actual validation compares the contents (bytes) of the certificate chain and not the hashes

MD5 is never used with TLS, and MD5 as a hash algorithm for certificates is no longer allowed either.

[zdohnal]

@zdohnal What uses of MD5 are you concerned about in CUPS?

AFAIK, none of the remaining uses of MD5 matter:

* MD5 for Digest authentication is a requirement unless the server/printer supports SHA2 (which we detect and use automatically)

You're right, I missed the first if block, where SHA2 can be used.

* MD5 for UUID generation is OK because there is no way to do prefix attacks and we don't use a lot of bits anyways

Seems good to me too.

* MD5 for showing a digest of a certificate chain to the user is purely informational - the actual validation compares the contents (bytes) of the certificate chain and not the hashes

You're right - I did not look properly - output of httpCredentialsString() where credentials are hashed is used only for info and for better comparison in httpCredentialsGetTrust().

MD5 is never used with TLS, and MD5 as a hash algorithm for certificates is no longer allowed either.

Unfortunately even those MD5 will not work if more strict crypto policy is set in gnutls, then gnutls_hash_fast() for MD5 will end with failure. This can be prevented with GNUTLS_FIPS140_SET_LAX_MODE(); and GNUTLS_FIPS140_SET_STRICT_MODE(); (more info on gnutls web https://gnutls.org/manual/html_node/FIPS140_002d2-mode.html ), which I plan to use for non-crypto use cases.

Would you be interested in such patch? I can create configure option for it if you would prefer the configuration option instead of putting it directly into the code, please let me know.

[michaelrsweet]

Or I can just use the old L. Peter Deutsch code when compiled against GNU TLS...

[michaelrsweet]

IMHO having GNU TLS disable a core crypto function rather than disabling it at a higher level is totally the wrong approach to take. They will just break working code without fixing anything...

[zdohnal]

Or I can just use the old L. Peter Deutsch code when compiled against GNU TLS...

Unfortunately then the project would lose the advantage of crypto library taking care of hash implementation, which in my opinion is more secure than have an internal implementation. And it spares your time for other stuff.

IMHO having GNU TLS disable a core crypto function rather than disabling it at a higher level is totally the wrong approach to take. They will just break working code without fixing anything...

It was just an example - in reality it will be forbidden on kernel level, according which crypto libraries will set up its cipher suites (IIUC).
The patch would cover allowing usage of MD5 for non-crypto cases inside the environment which disables MD5 and it would not lose its coverage of MD5 implementation by GNUTLS, so CUPS can run such systems with GNUTLS coverage. Would you be interested in such patch?

[michaelrsweet]

@zdohnal I’ll just need to see how invasive the patch is to say whether we’ll take it. But in the case of MD5 we’ve had L Peter’s code in CUPS forever...

[zdohnal]

@michaelrsweet Basically I plan to add those two lines mentioned above:

GNUTLS_FIPS140_SET_LAX_MODE(); and GNUTLS_FIPS140_SET_STRICT_MODE();

into functions where MD5 is used for non-crypto purposes:

cgi_set_sid()
httpAssembleUUID()
httpCredentialsString()

I can make it as new stub function, which will be called in those functions and it will take the name of current 'cupsHashData' - it will have the same parameters, so there will be no problem with changing API. The new stub function will call those two lines only when CUPS is linked with gnutls and it will call current cupsHashData function, but renamed to 'cupsHashData2()'.
If you would like me to write in the different way, please let me know.

[michaelrsweet]

@zdohnal Seems like the correct solution here would be to add these calls in the cupsHashData function itself.

[michaelrsweet]

(with a corresponding configure check, of course)

[zdohnal]

Hi Mike!
I created pull request for the issue #5620 . I tested cookie and UUID creation and it works.
I chose to create new cupsHashNoCryptoData() function as stub over cupsHashData for md5 algorithm, because if I put those relaxing calls into cupsHashData itself, it will allow to use MD5 even for digest auth (if server/printer does not have SHA2). Failing to authenticate against such machines is correct behavior when MD5 is used and GNU TLS forbids to use MD5 (according Nikos Mavrogiannopoulos, our gnutls upstream person).
Next I introduced --enable-gnutls-relax-mode configure option, which will enable MD5 in cupsHashData call for those non-crypto use cases (creation of cookie, UUID, digest of certchain for info). It is disabled by default (since not many distributions have forbidden MD5 yet) and can be applied only when CUPS is built with GNU TLS.
How does it look? Please let me know if I should do differently or what should I enhance. Would it be acceptable to have it in CUPS?

[michaelrsweet]

Ok so we won’t accept a patch like that.

Local or printing is different from usage over the internet. For this reason, all of the cert and cipher suite policies are managed separately by CUPS so that appropriate policies can be assigned for local, “IoT” devices that are different from what you’d use for cloud services and web browsers. Moreover, CUPS always prefers the most secure hash algorithms and cipher suites when communicating with a printer. If a printer supports something better than MD5 or SHA1 we will use it.

If there is an API we can call to obtain specific policy choices for local printing, we will be happy to use it to set the defaults vs the current client.conf file stuff, but we will not break printing to satisfy the whims of someone that is focused solely on internet security.

[zdohnal]

I see what you mean and understand you point, I'll check it up with our crypto people what can be done.

[zdohnal]

According our crypto team there is an initiative for a tool which will allow to use specific cipher in specific program... so it can result in the state that we do not have to concern about how strict/relax gnutls is in CUPS and user can set it up in other app.
Until this feature is available, would you mind considering the pull request which is based on what you suggested before?

[michaelrsweet]

@zdohnal We will consider the new pull request (#5622) but I'd really like it to just be a configure test - if the function is there then use it, otherwise don't bother.

[michaelrsweet]

Changes pushed to 2.2.x and 2.3.x. Please review and post any feedback to the pull request (#5622).

[zdohnal]

LGTM and it covers the problematic use case. Thank you, Mike!