1292914 - libssl doesn't check first padding byte if there is 255-bytes of padding

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Eric Rescorla (:ekr)]

The padding check in ssl_RemoveTLSCBCPadding checks a maximum of 255 bytes of padding but then checks the bytes starting with the final byte (plaintext->len -1 - i) which is the padding length byte. You need to check all the padding bytes + the padding length byte == 256. This creates a problem whenever the other side has (a) maximal padding and (b) the content is block aligned.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Fix plus test cases

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8778598 [details] [diff] [review]
Fix plus test cases

Review of attachment 8778598 [details] [diff] [review]:
-----------------------------------------------------------------

MT, please review whole patch.
WTC, please review the changes to ssl3con.c

Comment 3

[Eric Rescorla (:ekr)]

Found with BoGo test harness.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8778598 [details] [diff] [review]
Fix plus test cases

Review of attachment 8778598 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +80,4 @@
>                                     const unsigned char *additionalData,
>                                     int additionalDataLen);
>  #endif
> +SECStatus ssl_RemoveTLSCBCPadding(sslBuffer *plaintext, unsigned int macSize);

It is not necessary to add this declaration here.

I probably would add the declaration to sslimpl.h, so that
ssl_record_unittest.cc doesn't need to manually declare
this function.

@@ +13038,4 @@
>       * decrypted information. Therefore we always have to check the maximum
>       * amount of padding possible. (Again, the length of the record is
>       * public information so we can use it.) */
> +    toCheck = 256; /* maximum amount of padding. */

As we discussed over chat, I think we should fix this bug by changing

        unsigned char b = plaintext->buf[plaintext->len - 1 - i];

to

        unsigned char b = plaintext->buf[plaintext->len - 2 - i];

So that we don't check the padding_length byte again.

Comment 5

[Eric Rescorla (:ekr)]

OK, I think I've figured out what's happening here. The basic strategy of this
code is as follows (ignoring the detailed bit manipulations):

  0. Set |good| to 1 if the padding length byte is consistent with the
     mininum fixed data in the record (i.e., pad_len + mac_len + 1 <= len),
     otherwise all 0s.
     
  1. Iterate over a fixed number of bytes that is as long as any
     possible padding, starting from the padding length byte. This
     involves checking some non-padding bytes, so you ignore those
     with a mask.
     
     1. Set a mask to all 1s if the byte is padding and all 0s otherwise.
     2. We accumulate in |good| by taking the XOR of each byte and |pad_len|
        (which is 0 if they match) and filtering it through the mask, so that
        if the mask is all 1s (i.e., it's padding) and there is a padding
        mismatch, then some bits in |good| are cleared
        
  2. At the end of this process, the bottom byte of |good| is either all 1s
     in which case the padding is OK, or something else in which case it's
     bad.

This code generally works, except that there is an off-by-one error in step #1,
which sets the maximum number of bytes to check at 255. However, because we
are starting from the length byte, there are actually 256 bytes to check
(the padding length byte + 255 bytes of pad).

There are two potential fixes:

1. The one in this patch, which is to just change 255 to 256, thus checking
   all the bytes.
   
2. To slide the check over to the left by one byte so you don't
   check the padding length, which you know is equal to itself,
   as WTC suggests. i.e., changing

        unsigned char b = plaintext->buf[plaintext->len - 1 - i];
   to
        unsigned char b = plaintext->buf[plaintext->len - 2 - i];   

   However, to do this, you need to also change the mask computation
   because otherwise when the padding is less than maximal, the mask
   is nonzero for the last non-pad byte. You do that by changing
   the mask computation as well:

        unsigned int t = paddingLength - i - 1;


There is still one remaining question, which is the original |toCheck| value,
which is set to MIN(255, plaintext->len - 1). This means that if you have
a record that is 256 bytes or less long and consists entirely of padding,
you don't check the first padding byte. Of course, this is an impossible case
because in TLS there are always at least macSize bytes of MAC in any record,
and so you actually fail the initial consistency check in step 0. However,
this suggests that maybe the plaintext->len - 1 step is over-checking and
we should be removing all of macSize. It's not a serious issue, but I wanted
to see what people thought.

Comment 6

[Eric Rescorla (:ekr)]

Adding agl to CC in case he has comments.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8778598 [details] [diff] [review]
Fix plus test cases

Review of attachment 8778598 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +13042,3 @@
>      if (toCheck > plaintext->len - 1) {
>          toCheck = plaintext->len - 1;
>      }

Eric: I just studied this code independently and came to the same
conclusions (except I didn't notice the overchecking). If we think
the CBC block cipher suites are becoming less important in practice,
then I suggest we go for the smallest fix; we should also update
the comment:

    toCheck = 256; /* maximum amount of padding + 1. */

We check the padding_length byte redundantly, but the comments
"i <= paddingLength" and "the final |paddingLength+1| bytes"
show this is the intent of the code.

I also recommend fixing the if statement. Either just prevent
going past the |plaintext| buffer boundary:
    if (toCheck > plaintext->len) {
        toCheck = plaintext->len;
    }

or fix overchecking:

    if (toCheck > plaintext->len - macSize) {
        toCheck = plaintext->len - macSize;
    }

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8778598 [details] [diff] [review]
Fix plus test cases

Review of attachment 8778598 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/ssl_gtest/ssl_record_unittest.cc
@@ +13,5 @@
> +namespace nss_test {
> +
> +// References to "internal" code in libssl
> +extern "C" {
> +  SECStatus ssl_RemoveTLSCBCPadding(sslBuffer *plaintext, unsigned int macSize);

You should just put this in libssl_internals.h with a note about it being defined in ssl???.c  Or take Wan-Teh's suggestion.

@@ +21,5 @@
> +      public ::testing::Test,
> +      public ::testing::WithParamInterface<std::tuple<size_t, bool>> {
> + public:
> +  TlsPaddingTest() :
> +      ct_len_(std::get<0>(GetParam())) {

I thought that ct_len_ == cipher text length.  Rename please.

@@ +32,5 @@
> +    }
> +    MakePaddedPlaintext('A', ct_len_, &plaintext_);
> +    std::cerr << "Content length=" << ct_len_
> +              << " padding length=" << pad_len_
> +              << " total length=" << plaintext_.len()

Maybe it's better to write these out in Unpad().

@@ +39,5 @@
> +
> +  // Makes a plaintext record with correct padding.
> +  void MakePaddedPlaintext(uint8_t content,
> +                           size_t length,
> +                           DataBuffer *d) {

The structure here is odd.  You calculate the content length and padding in the constructor, and those are members.  But then you pass the ct_len_ and plaintext_ members to this function.  pad_len_ isn't passed in though.

@@ +72,5 @@
> +  size_t ct_len_;
> +  size_t pad_len_;
> +  DataBuffer plaintext_;
> +};
> +

I think that you forgot the success case.

@@ +74,5 @@
> +  DataBuffer plaintext_;
> +};
> +
> +TEST_P(TlsPaddingTest, PadTooLong) {
> +  DataBuffer d;

unused (and below).

Comment 9

[Adam Langley]

Sorry, this is my fault. Peter Gijsels pointing this out to me some time ago and he should get the credit for it:

https://boringssl.googlesource.com/boringssl/+/80842bdb44855dd7f1dde64a3fa9f4e782310fc7%5E%21/

Sadly I misunderstood him and thought that he was going to do the rounds of reporting it to various projects when, upon rereading the email, it can (and should) be read the other way around. Since it doesn't appear to have much impact, I didn't chase it up. (And, now that I look back, he sent me another email about an optimisation that I should consider.)

I believe this issue is also in OpenSSL and I'm going to send them a patch for it today.

Comment 10

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1292914-Correctly-strip-TLS-CBC-padding-when-the.patch

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8778959 [details] [diff] [review]
0001-Bug-1292914-Correctly-strip-TLS-CBC-padding-when-the.patch

Review of attachment 8778959 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/ssl_gtest/ssl_record_unittest.cc
@@ +51,5 @@
> +    sslBuffer s;
> +    s.buf = const_cast<unsigned char *>(
> +        static_cast<const unsigned char *>(plaintext_.data()));
> +    s.len = plaintext_.len();
> +    SECStatus rv = ssl_RemoveTLSCBCPadding(&s, 20);

I think that you should make the 20 a constant.  Mainly because it helps document its reason for existence.

@@ +84,5 @@
> +
> +TEST_P(TlsPaddingTest, FirstByteOfPadWrong) {
> +  if (pad_len_) {
> +    plaintext_.Write(plaintext_len_,
> +                     plaintext_.data()[plaintext_len_] ^ 0xff, 1);

Maybe try an off-by one in one of these tests rather than flipping all the bits.

@@ +97,5 @@
> +    Unpad(false);
> +  }
> +}
> +
> +const static size_t kContentSizesArr[] = {1, 20, 30, 31, 32, 36,

Add kMacSize-1 (19) to guard against off-by-one on the MAC check.

@@ +98,5 @@
> +  }
> +}
> +
> +const static size_t kContentSizesArr[] = {1, 20, 30, 31, 32, 36,
> +                                          256, 257, 287, 288 };

spacing around the braces is inconsistent

::: lib/ssl/ssl3con.c
@@ +13014,3 @@
>  ssl_RemoveTLSCBCPadding(sslBuffer *plaintext, unsigned int macSize)
>  {
>      unsigned int paddingLength, good, t, toCheck, i;

It took me a while to work out what was going on, but there is name shadowing on `t` in this function.  I don't want to juberti you, but it is bad.

Comment 12

[Wan-Teh Chang]

Comment on attachment 8778959 [details] [diff] [review]
0001-Bug-1292914-Correctly-strip-TLS-CBC-padding-when-the.patch

Review of attachment 8778959 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to update the comment.

Cleaning up this function (such as fixing the name shadowing of
the variable 't' and fixing the over-checking) would make a good
project for new people like Franziskus and Tim, if they have the
interest and time. I think the cleanup is worthwhile.

::: lib/ssl/ssl3con.c
@@ +13037,4 @@
>       * decrypted information. Therefore we always have to check the maximum
>       * amount of padding possible. (Again, the length of the record is
>       * public information so we can use it.) */
> +    toCheck = 256; /* maximum amount of padding. */

Please update this comment.

  toCheck = 256; /* maximum amount of padding + 1. */

or use Adam's comment from BoringSSL:

  toCheck = 256; /* maximum amount of padding, inc length byte. */

Comment 13

[Eric Rescorla (:ekr)]

Fixed as: https://hg.mozilla.org/projects/nss/rev/e3c5c0541231

Comment 14

[Eric Rescorla (:ekr)]

Matt, can you unhide please.