Support full uint range for body length

[emoney-mli]

Describe your changes

Context

• eMoney Advisor receives large pgp encrypted files from financial institutions.
• With one institution, we have one file (out of 8 or so total that they send to us) that randomly fails when being decrypted by BouncyCastle, giving a EndOfStreamException, saying "Attempted to read past end of stream"
• the failing file is ~4.1GB
• The institution has confirmed that their encryption method is global across all files, so this singular file that is failing should not be any different
• I have eliminated any in between code and am running the bad file (and good ones, for comparison) directly through the BouncyCastle code to see what is happening. Details to follow.
• We have not been able to create a file that behaves the same way. We manually force decrypt the breaking file using gpg directly and re-encrypt it to get it to work, and BouncyCastle processes that file successfully.

The Bug

This is what I am seeing for the failing file. Everything goes fine up until we hit the literal data packet piece of the pgp file. The following screenshots are from when we start processing the literal data packet.

The first byte of the header shows that it is a long length packet (b0 = 255). Note that in this case, the header length is 6 bytes, so after this first byte, we have 5 to read still. Also, I have changed the code ever so slightly in the screenshots below just so I could see the numbers at each step--no functionality changes were made though.

If I step into the RequireUInt32BE() method, I get the following:

Where you can see that the bytes of the header are properly read and give us 4082119084, which if you refer back to the packet screenshot above, corresponds exactly with the packet length (plen).

We now return back to the ReadBodyLen method where we started with this as the results

The response variable lines up with what we see as the packet length from the previous step. However, the code then casts this UInt as an int, which results in an overflow, and we get a negative number. Again, I have some extra variables here just to highlight the difference in values in each step of the process.

This negative number goes back to the calling method, RequireBodyLen(), which then throws an error because the body length was negative.

However, I don’t believe the EndOfStream exception is even accurate here because, from what I can tell, we did not read past the end of the stream. We were simply reading header bytes to determine the length of the literal packet content that follows the header.

According to the PGP spec 4880 this large size is not a violation of the rules, so I would expect it to work (and based on debugging the BouncyCastle code it seems totally fine reading the headers and getting the correct length value until it tries to cast it as an int). The problem is that the file length is simply too big to read into an int, and as marked by this TODO, BouncyCastle needs to support UInt so that large files can be processed.

The Fix

This pull request modifies StreamUtilities.cs ReadBodyLen method to return a uint? instead of a int. If the length is invalid for any reason, it returns null, otherwise it returns the proper size represented in the uint range. All other file changes are either to make the compiler happy or to enforce that partial data packets have a max size of 2**30 as per the spec, even though the uint range can support up to 2**32 - 1.

How has this been tested?

The bc-test-data GitHub repository was cloned and all tests were ran in the test folder and successfully passed.

Checklist before requesting a review

• I have performed a self-review of my code
• I have kept the patch limited to only change the parts related to the patch
• This change requires a documentation update

See also Contributing Guidelines.

[peterdettman]

Thanks for the PR, however this was already fixed based on an emailed bug report. I will review in case we missed something.

[emoney-mli]

Hi Peter, thank you! Do you have an estimate as to when the future release containing this bug fix will be out? Thank you!

[emoney-mli]

I was also wondering if the Java version of bouncycastle has the same issue

[peterdettman]

Probably we will publish a 2.6.2 in the next week or so.

The Java version was reviewed and it was found to already have special handling for large/"negative" values, despite storing the length in an int.

[emoney-mli]

Interesting, if it's storing the length in an int, isn't it still subject to memory limit of ~2bil max value?

[peterdettman]

Although stored as int, all operations/comparisons treat it as an unsigned value. Please review the BCPGInputStream code for details.

[emoney-mli]

Gotcha thank you!

[emoney-mli]

Hi @peterdettman , I was wondering when 2.6.2 will be released so we can upgrade! Thank you

[peterdettman]

It's actually already up (https://www.nuget.org/packages/BouncyCastle.Cryptography/2.6.2), we are just preparing the various readme/website/github updates.

[emoney-mli]

Thank you! Appreciate it