1344380 - (CVE-2017-5461) Write beyond bounds caused by bugs in Base64 de/encoding in nssb64d.c and nssb64e.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[q1]

PL_Base64MaxDecodedLength() (security\nss\lib\util\nssb64d.c) can cause an integer overflow. If it does, its caller(s) will allocate a buffer too small for the result of a Base64 decoding operation, then possibly overrun it.

For example, PL_Base64DecodeBuffer() (security\nss\lib\util\nssb64d.c) uses the result of PL_Base64MaxDecodedLength() to check whether a caller-provided buffer is long enough, or to allocate a buffer if the caller does not provide one. In either case, the calculated length is inadequate if the length of the encoded string is >= 0x55555556. Once it has calculated the (potentially incorrect) length, it passes it and the corresponding buffer to pl_base64_decode_buffer() (same module) to do the decoding. That function incrementally checks against the provided length (data->output_buflen) but (another bug!) *only in debug builds (i.e. via a PR_ASSERT())*.

The upshot is that PL_Base64DecodeBuffer() might be vulnerable to the same kind of buffer overrun bug as in https://bugzilla.mozilla.org/show_bug.cgi?id=1344081 : an attack that overruns a heap buffer, by an attacker-determined amount, with 3-byte granularity.

There is a similar issue in PL_Base64MaxEncodedLength() and its associated functions for Base64 encoding.

I will try to manifest these bugs and post a POC if and when I do.

The bugs are in lines 373 and 254 (nssb64d.c):

370: static PRUint32
371: PL_Base64MaxDecodedLength (PRUint32 size)
372: {
373:     return ((size * 3) / 4);
374: }

219: static PRStatus
220: pl_base64_decode_buffer (PLBase64Decoder *data, const unsigned char *in,
221: 			 PRUint32 length)
222: {
223:     unsigned char *out = data->output_buffer;
224:     unsigned char *token = data->token;
225:     int i, n = 0;
226: 
227:     i = data->token_size;
228:     data->token_size = 0;
229: 
230:     while (length > 0) {
231: 	while (i < 4 && length > 0) {
...
254: 	   PR_ASSERT((out - data->output_buffer + 3) <= data->output_buflen);

and 288 and potentially 300 (nssb64e.c):

280: static PRUint32
281: PL_Base64MaxEncodedLength (PRUint32 size, PRUint32 line_length)
282: {
283:     PRUint32 tokens, tokens_per_line, full_lines, line_break_chars, remainder;
284: 
285:     tokens = (size + 2) / 3;
286: 
287:     if (line_length == 0)
288: 	      return tokens * 4;
289: 
290:     if (line_length < 4)	/* too small! */
291: 	      line_length = 4;
292: 
293:     tokens_per_line = line_length / 4;
294:     full_lines = tokens / tokens_per_line;
295:     remainder = (tokens - (full_lines * tokens_per_line)) * 4;
296:     line_break_chars = full_lines * 2;
297:     if (remainder == 0)
298: 	   line_break_chars -= 2;
299: 
300:     return (full_lines * tokens_per_line * 4) + line_break_chars + remainder;
301: }

Comment 1

[q1]

"There is a similar issue in PL_Base64MaxEncodedLength() and its associated functions for Base64 encoding" should be followed by " (nssb64e.c)".

Comment 2

[Frederik Braun [:freddy]]

Adding Fransziskus, an NSS peer, per his request.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://nss-review.dev.mozaws.net/D256

Comment 4

[Daniel Veditz [:dveditz]]

This seems to be used a bunch in our codebase. Bug 1344081 and bug 1344305 are due to this, but it's also used as the basis for Base64Decode() in xpcom/io which seems to be used all over the place.

Comment 5

[Daniel Veditz [:dveditz]]

https://searchfox.org/mozilla-central/search?q=Base64Decode&path=

Comment 6

[Eric Rahm [:erahm]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> This seems to be used a bunch in our codebase. Bug 1344081 and bug 1344305
> are due to this, but it's also used as the basis for Base64Decode() in
> xpcom/io which seems to be used all over the place.

Base64Decode() in xpcom/io uses NSPR's base64 functions, not NSS's.

Comment 7

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/77a5bb81dbaac5b03266a64ff981c156b61c8931

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I assume we'll want to backport this to 3.28/3.29/3.30?

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as affected for current versions and esr. Should this be included in whatever NSS update we may do across all branches?

Comment 10

[Franziskus Kiefer [:franziskus]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> I assume we'll want to backport this to 3.28/3.29/3.30?

If there's another 3.28 point release, we'll add this bug (see bug 1344368). As for 3.29 and 3.30, we'll wait another two weeks to see if there's more we have to get in. (Leaving the ni? here as a reminder.)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> If there's another 3.28 point release, we'll add this bug (see bug 1344368).

This bug would be enough to be a driver for another 3.28 point release, wouldn't it? Need to figure out what to do about ESR45 as well, which is on 3.21.3 at the moment.

Comment 12

[q1]

> passes it and the corresponding buffer to pl_base64_decode_buffer() (same module) to do the decoding. That function
> incrementally checks against the provided length (data->output_buflen) but (another bug!) *only in debug builds (i.e. via a 
> PR_ASSERT())*.

The patch appears not to fix this portion of the original bug.

Comment 13

[Franziskus Kiefer [:franziskus]]

> This bug would be enough to be a driver for another 3.28 point release, wouldn't it?

Yeah, we might make a point release based on this one. But that wouldn't get picked up by Firefox as long as there's no point release for 52.

> >  pl_base64_decode_buffer(). That function incrementally checks against the provided length (data->output_buflen) only in debug builds
> The patch appears not to fix this portion of the original bug.

It only asserts, that's correct. But with PL_Base64MaxDecodedLength computing the correct length this really is a sanity check. Do you have inputs that trigger this assert?

Comment 14

[q1]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> > >  pl_base64_decode_buffer(). That function incrementally checks against the provided length (data->output_buflen) only in debug builds
> > The patch appears not to fix this portion of the original bug.
> 
> It only asserts, that's correct. But with PL_Base64MaxDecodedLength
> computing the correct length this really is a sanity check....

OK, I see: pl_base64_decode_buffer() is |static|, so it's only ever called by other functions in nssb64d.c, and both current callers call PL_Base64MaxDecodedLength() first. So it's OK.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Landed on the NSS 3.28.4 branch:

https://hg.mozilla.org/projects/nss/rev/6eb39ead39e0

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> Landed on the NSS 3.28.4 branch:
> 
> https://hg.mozilla.org/projects/nss/rev/6eb39ead39e0

Are we taking this in Firefox 53?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Al Billings [:abillings] from comment #16)
> (In reply to Tim Taubert [:ttaubert] from comment #15)
> > Landed on the NSS 3.28.4 branch:
> > 
> > https://hg.mozilla.org/projects/nss/rev/6eb39ead39e0
> 
> Are we taking this in Firefox 53?

Yeah. Franziskus is currently preparing releases and uplift requests.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

This should be fixed in beta by the NSS upgrade in bug 1353740 (for the beta 10 build tomorrow)

Comment 19

[Kai Engert (:KaiE:)]

Will you assign a CVE?

Comment 20

[Huzaifa Sidhpurwala]

What is the trigger here? Base64 user input, like certificates, private key material or something else?

Comment 21

[Franziskus Kiefer [:franziskus]]

The trigger can be anything that's handed to the base64 decoder/encoder. So yes, also certs etc.

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi, can you tell us if this patch addresses the issue sufficiently?

Comment 23

[q1]

(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #22)
> Hi, can you tell us if this patch addresses the issue sufficiently?

Yes, the patch appears correct.

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

Thank you very much.