Re: Fix for CVE-2020-25648 in nss

[Roberto C. Sánchez <roberto@debian.org>]

Hi Sean,

On Fri, Oct 27, 2023 at 04:08:36PM +0100, Sean Whitton wrote:
> [attempt to resend to correct list address]
> 
> Hello,
> 
> I am trying to backport the fix for CVE-2020-25648 to nss version 3.42.1
> in Debian 10, "buster".  Unfortunately, the four tests added by the fix
> do not pass.
> 
> Would someone be able to take a look at the test output, please?
> Perhaps the failed test does not indicate that the fix is ineffective.
> As all the other tests pass, I'm not too concerned about introducing any
> regressions.
> 

It seems your backported patch might be faulty. The upstream patch [0]
contains the following change:

             ss->ssl3.hs.ws != idle_handshake &&
             cText->buf->len == 1 &&
             cText->buf->buf[0] == change_cipher_spec_choice) {
-            /* Ignore the CCS. */
-            return SECSuccess;
+            if (ss->ssl3.hs.allowCcs) {
+                /* Ignore the first CCS. */
+                ss->ssl3.hs.allowCcs = PR_FALSE;
+                return SECSuccess;
+            }
+
+            /* Compatibility mode is not negotiated. */
+            alert = unexpected_message;
+            PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
         }

         if ((IS_DTLS(ss) && !dtls13_AeadLimitReached(spec)) ||

However, your patch has this instead:

             cText->buf->buf[0] == change_cipher_spec_choice) {
             /* Ignore the CCS. */
             return SECSuccess;
+            if (ss->ssl3.hs.allowCcs) {
+                /* Ignore the first CCS. */
+                ss->ssl3.hs.allowCcs = PR_FALSE;
+                return SECSuccess;
+            }
+
+            /* Compatibility mode is not negotiated. */
+            alert = unexpected_message;
+            PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
         }

         if (IS_DTLS(ss) ||

It appears that the code which was added is never reached because the
unconditional 'return SECSuccess;' was not pushed down into the new 'if'
statement.

I tried a few times to run the tests, but I couldn't manage it. So, my
theory is untested, but based on the fact that your failing test output
includes

Got error code (null) expecting SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER

Since it seems like the function is returning SECSuccess and that that
added code is where the SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER value is
set, all of that makes me think that my theory is a likely explanation
for your unexpected test failure.

Regards,

-Roberto

[0] https://hg.mozilla.org/projects/nss/rev/57bbefa793232586d27cee83e74411171e128361

-- 
Roberto C. Sánchez