Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

({sec-low})

Trunk
mozilla52
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
derived from bug 1283710.

PR_GetErrorText() does nothing if PR_GetErrorTextLength() is 0.
https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/nsprpub/pr/src/misc/prerror.c#67
> PR_IMPLEMENT(PRInt32) PR_GetErrorText(char *text)
> {
>     PRThread *thread = PR_GetCurrentThread();
>     if (0 != thread->errorStringLength)
>         memcpy(text, thread->errorString, thread->errorStringLength+1);
>     return thread->errorStringLength;
> }  /* PR_GetErrorText */


If the caller doesn't zero the buffer, it results in uninitialized unterminated string.

https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/js/src/ctypes/Library.cpp#158
>     char* error = (char*) JS_malloc(cx, PR_GetErrorTextLength() + 1);
>     if (error)
>       PR_GetErrorText(error);


so far I don't see any other critical case tho, marking as s-s just in case.
(Assignee)

Comment 2

3 years ago
maybe I should file another bug for this tho, posting here as it's adjacent code to Part 1, and somewhat related.

`error` there is OS error, that's not guaranteed to be ASCII or UTF-8.
Now we can check the encoding by StringIsASCII, added Latin1 branch for non-ascii case.
Attachment #8802474 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
Comment on attachment 8802471 [details] [diff] [review]
Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0.

sorry, this was wrong one.
will post correct one shortly.
Attachment #8802471 - Attachment is obsolete: true
Attachment #8802471 - Flags: review?(sphink)
(Assignee)

Comment 4

3 years ago
Changed to use the same way as other case.
now it allocates string on stack with default message, and overwrites only if there's OS error and it fits in the string.
Attachment #8802491 - Flags: review?(sphink)
(Assignee)

Comment 5

3 years ago
rebased, and fixed JS:: namespace
Attachment #8802474 - Attachment is obsolete: true
Attachment #8802474 - Flags: review?(jwalden+bmo)
Attachment #8802492 - Flags: review?(jwalden+bmo)
Attachment #8802491 - Flags: review?(sphink) → review+
(In reply to Tooru Fujisawa [:arai] from comment #0)
> so far I don't see any other critical case tho, marking as s-s just in case.

I'm going to mark this bug sec-audit because of this. If you did find some bad cases, please adjust the rating or clear it and we'll look at it in triage again.
Keywords: sec-audit
(Assignee)

Comment 7

3 years ago
found 2 other cases that may hit this issue.

https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/testing/mochitest/ssltunnel/ssltunnel.cpp#1560-1564
>     int32_t errorlen = PR_GetErrorTextLength();
>     char* err = new char[errorlen+1];
>     PR_GetErrorText(err);
>     LOG_ERROR(("Failed to init NSS: %s", err));
>     delete[] err;

https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/nsprpub/pr/tests/errset.c#138-149
>         char    msg[256];
> 
>         PR_SetError( errnum, -5 );
>         err = PR_GetError();
>         PR_ASSERT( err == errnum );
>         err = PR_GetOSError();
>         PR_ASSERT( err == -5 );
>         PR_SetErrorText( strlen(errcodes[errnum].errname), errcodes[errnum].errname );
>         len1 = PR_GetErrorTextLength();
>         len2 = PR_GetErrorText( msg );
>         PR_ASSERT( len1 == len2 );
>         printf("%5.5d -- %s\n", errnum, msg );

according to the path, both are testing code.
(Assignee)

Comment 8

3 years ago
js/src/ctypes/Library.cpp's case will be sec-low:
  * chrome-priv is necessary to call this function
  * this is in error handling, and web content cannot cause the error
  * this can expose uninitialized memory content as error message string,
    but the error message isn't available to web content

nsprpub/pr/tests/errset.c wasn't a bug.  the text is set by the code and the length is always non-zero

I'm not sure about testing/mochitest/ssltunnel/ssltunnel.cpp
Keywords: sec-auditsec-low
(Assignee)

Comment 9

3 years ago
oops, changed by mistake.
still need audit for ssltunnel.cpp
Keywords: sec-lowsec-audit
(In reply to Tooru Fujisawa [:arai] from comment #9)
> still need audit for ssltunnel.cpp

Note the comment atop ssltunnel.cpp:

/*
 * WARNING: DO NOT USE THIS CODE IN PRODUCTION SYSTEMS.  It is highly likely to
 *          be plagued with the usual problems endemic to C (buffer overflows
 *          and the like).  We don't especially care here (but would accept
 *          patches!) because this is only intended for use in our test
 *          harnesses in controlled situations where input is guaranteed not to
 *          be malicious.
 */

Whatever problems ssltunnel.cpp might or might not have, effectively they're in test-only code and should be deprioritized accordingly.
(Assignee)

Comment 11

3 years ago
Thanks!
will prepare a patch for ssltunnel.cpp.
Keywords: sec-auditsec-low
Comment on attachment 8802492 [details] [diff] [review]
Part 2: Use Latin1 variant of error reporting if OS error is not ASCII.

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

Nice approach!
Attachment #8802492 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8803223 [details] [diff] [review]
Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp.

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

::: testing/mochitest/ssltunnel/ssltunnel.cpp
@@ +1560,5 @@
>      int32_t errorlen = PR_GetErrorTextLength();
> +    if (errorlen) {
> +      char* err = new char[errorlen+1];
> +      PR_GetErrorText(err);
> +      LOG_ERROR(("Failed to init NSS: %s", err));

Do

  auto err = mozilla::MakeUnique<char[]>(errorlen + 1);
  PR_GetErrorText(err.get());
  LOG_ERROR(("Failed to init NSS: %s", err.get()));

so you can remove the delete[].  (Technically this is less efficient, because MakeUnique value-initializes the array, which for scalar elements zero-initializes them.  But it's test code, who cares.  :-) )
Attachment #8803223 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1be47494e723521e96f5541d985c63a77dd225ef
Bug 1311319 - Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0. r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd94a51349c104067c7dbd7b7f76d90c95fda0b
Bug 1311319 - Part 2: Use Latin1 variant of error reporting if OS error is not ASCII. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c77ff67311dd0cf52bc211ce0cb1d118000f3d4
Bug 1311319 - Part 3: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0 in ssltunnel.cpp. r=jwalden
(Assignee)

Comment 17

3 years ago
Part 2 don't need backport.
Part 1 and 3 might be nice to backport to beta and aurora.
will check and prepare.
(Assignee)

Comment 18

3 years ago
Approval Request Comment
> [Feature/regressing bug #]
Bug 1131424 (Part 1) and Bug 426867 (Part 3).

> [User impact if declined]
Possible crash.

> [Describe test coverage new/current, TreeHerder]
tested in automated test.

> [Risks and why]
Low.  Both add fallback message for erroneous case.

> [String/UUID change made/needed]
None
Attachment #8803717 - Flags: review+
Attachment #8803717 - Flags: approval-mozilla-beta?
Attachment #8803717 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8803718 - Flags: review+
Attachment #8803718 - Flags: approval-mozilla-beta?
Attachment #8803718 - Flags: approval-mozilla-aurora?
Group: core-security → core-security-release
Comment on attachment 8803717 [details] [diff] [review]
(mozilla-aurora/beta) Part 1: Do not call PR_GetErrorText() when PR_GetErrorTextLength() is 0. r=sfink

This is a sec-low, ok for Aurora51, wontfix for beta50.
Attachment #8803717 - Flags: approval-mozilla-beta?
Attachment #8803717 - Flags: approval-mozilla-beta-
Attachment #8803717 - Flags: approval-mozilla-aurora?
Attachment #8803717 - Flags: approval-mozilla-aurora+
Attachment #8803718 - Flags: approval-mozilla-beta?
Attachment #8803718 - Flags: approval-mozilla-beta-
Attachment #8803718 - Flags: approval-mozilla-aurora?
Attachment #8803718 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.