[Static Analysis][Uninitialized pointer read] Function main from mar.c can produce a bad access on pointer DERFilePaths

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
Application Update
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

(Whiteboard: CID 1340242)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

2 years ago
The Static Analysis tool Coverity showed that a potentially bad access can result when dereferencing DERFilePaths.
(Assignee)

Comment 1

2 years ago
Created attachment 8692524 [details] [diff] [review]
Bug 1228332.diff

hello greg,

Could you pelase take a look other this patch and tell me what you think? It should resolve the issue mentioned by coverity.

THX
Attachment #8692524 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Whiteboard: CID 1340242

Updated

2 years ago
Component: Build Config → Application Update
Product: Core → Toolkit

Comment 2

2 years ago
Comment on attachment 8692524 [details] [diff] [review]
Bug 1228332.diff

I'm not an appropriate reviewer for this code. Redirecting.
Attachment #8692524 - Flags: review?(gps) → review?(robert.strong.bugs)
(Assignee)

Updated

2 years ago
Blocks: 1230156
Attachment #8692524 - Flags: review?(robert.strong.bugs) → review?(spohl.mozilla.bugs)
Comment on attachment 8692524 [details] [diff] [review]
Bug 1228332.diff

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

I'm assuming that the problem flagged by the static analysis tool is the call |fprintf(stderr, "ERROR: could not read file %s", DERFilePaths[k]);|. If not, please post what the static analysis tool flagged. Thanks!

::: modules/libmar/tool/mar.c
@@ +367,2 @@
>        if (rv) {
>          fprintf(stderr, "ERROR: could not read file %s", DERFilePaths[k]);

It's actually meaningless to print |DERFilePaths[k]| here if we didn't call |mar_read_entire_file| in the |
#if (defined(XP_WIN) || defined(XP_MACOSX)) && !defined(MAR_NSS)| block above. Please move this to the block with the |mar_read_entire_file| call. Also, add a |fprintf| and a |break| in the event that we can't find a cert from a nickname via |PK11_FindCertFromNickname|.
Attachment #8692524 - Flags: review?(spohl.mozilla.bugs) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8697538 [details] [diff] [review]
Bug 1228332.diff

I did the modifications and i kept the assert just to be safe:

>> assert(DERFilePaths[k]);

I know that DERFilePaths will be set here: 

>>    else if (argv[1][0] == '-' &&
>>             argv[1][1] == 'D' &&
>>             (argv[1][2] == (char)('0' + certCount) || argv[1][2] == '\0')) {
>>      if (certCount >= MAX_SIGNATURES) {
>>        print_usage();
>>        return -1;
>>      }
>>      DERFilePaths[certCount++] = argv[2];
>>      argv += 2;
>>      argc -= 2;
>>    }

Also i kept it to silence the analyzer.

Variable k can never be greater than MAX_SIGNATURES since it ranges from [0,certCount) and certCount is checked here:
>>     if (certCount >= MAX_SIGNATURES) {
>>       print_usage();
>>       return -1;
>>     }

If you have comments just let me know.
Attachment #8692524 - Attachment is obsolete: true
Attachment #8697538 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8697538 [details] [diff] [review]
Bug 1228332.diff

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

::: modules/libmar/tool/mar.c
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <assert.h>

let's drop the assert and this include.

@@ +142,5 @@
>  #endif
>  
> +#if !defined(NO_SIGN_VERIFY)
> +  memset(DERFilePaths, 0, sizeof(DERFilePaths));
> +#endif

You no longer need to move this down here. The |memset| happens in the correct #ifdef blocks.

@@ +345,5 @@
>      for (k = 0; k < certCount; ++k) {
>  #if (defined(XP_WIN) || defined(XP_MACOSX)) && !defined(MAR_NSS)
>        rv = mar_read_entire_file(DERFilePaths[k], MAR_MAX_CERT_SIZE,
>                                  &certBuffers[k], &fileSizes[k]);
> +      assert(DERFilePaths[k]);

|DERFilePaths[k]| has already been dereferenced on the previous two lines, so this assert isn't helping anything. Let's just drop it.

@@ +349,5 @@
> +      assert(DERFilePaths[k]);
> +      if (rv) {
> +          fprintf(stderr, "ERROR: could not read file %s", DERFilePaths[k]);
> +          break;
> +      }

nit: The indent here is wrong. Please make sure you're using spaces (not tabs) and indent by two spaces.

@@ +365,5 @@
>          certBuffers[k] = certs[k]->derCert.data;
>          fileSizes[k] = certs[k]->derCert.len;
>        } else {
>          rv = -1;
> +        fprintf(stderr, "ERROR: certs[k] is invalid");

Let's change this to:
fprintf(stderr, "ERROR: could not find cert from nickname %s", certNames[k]);

Also, you need to add a |break;| after the |fprintf|.
Attachment #8697538 - Flags: review?(spohl.mozilla.bugs) → review-
(Assignee)

Comment 6

2 years ago
Created attachment 8697560 [details] [diff] [review]
Bug 1228332.diff
Attachment #8697560 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8697560 [details] [diff] [review]
Bug 1228332.diff

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

Can you confirm that this satisfies the static analyzer? If yes, this is good to go. Thanks!
Attachment #8697560 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8697560 [details] [diff] [review]
Bug 1228332.diff

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

Actually, the commit message needs to be updated. Almost there! ;-)

Something like this should do:

Bug 1228332 - Fix a potentially unsafe pointer dereference, flagged by static analysis. r=spohl
Attachment #8697560 - Flags: review+ → review-
Attachment #8697538 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Hello Stephen,

I don't think the static analyzer it will be OK with the current patch since as you said earlier DERFilePaths is already dereferenced on the line bellow:

>>      rv = mar_read_entire_file(DERFilePaths[k], MAR_MAX_CERT_SIZE,
>>                                &certBuffers[k], &fileSizes[k]);

>>      if (rv) {
>>        fprintf(stderr, "ERROR: could not read file %s", DERFilePaths[k]);
>>        break;
>>      }

But if we go inside functions mar_read_entire_file we can see that filePath it's checked:

>>  if (!filePath || !data || !size) {
>>    return -1;
>>  }

Also we've talked earlier that is almost inposible for DERFilePaths[k] to be null for this line:

>> DERFilePaths[certCount++] = argv[2];

and  k < certCount

But Coverity can't figure this out so in order to silence it i won't remove that assert that we are talking about, so if you agree with me we can do this:

>>      assert(DERFilePaths[k]);
>>      rv = mar_read_entire_file(DERFilePaths[k], MAR_MAX_CERT_SIZE,
>>                                &certBuffers[k], &fileSizes[k]);

It is not that important where we place the assert function above or bellow mar_read_entire_file, for the static analyzer it's important to signal it that we don't expect a null value.

THX
(Assignee)

Updated

2 years ago
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 10

2 years ago
By the way same guard should be done on certNames, since in case of failure this is printed as well.
Hey Bogdan, could you copy here what Coverity is telling us? Both before any patches and after the current patch. I'd like to make sure there's no confusion about what we're trying to fix here. Thanks!
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 12

2 years ago
Created attachment 8698002 [details] [diff] [review]
Bug 1228332.diff
Attachment #8697560 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Sorry, now that we've confirmed this to be a uninitialized pointer read, let's improve the checkin comment to the following:
Bug 1228332 - Fix a potentially uninitialized pointer read, flagged by static analysis. r=spohl
Keywords: checkin-needed
(Assignee)

Comment 14

2 years ago
Created attachment 8698003 [details] [diff] [review]
Bug 1228332.diff

With description changed :)
Attachment #8698002 - Attachment is obsolete: true
Just for reference, this is what Coverity was complaining about:

1. DERFilePaths is a char* pointer, allocated on the stack with size MAX_SIGNATURES [1], which is currently 8 [2].
2. DERFilePaths is memset to 0, but only if the following is true: [3]
    #if !defined(NO_SIGN_VERIFY) && ((!defined(MAR_NSS) && defined(XP_WIN)) || defined(XP_MACOSX))
3. The dereference[4] of DERFilePaths in the fprintf may cause an uninitialized pointer read, because even though it happens inside an |#ifndef NO_SIGN_VERIFY| [5], it is dereferenced regardless of whether or not |#if (defined(XP_WIN) || defined(XP_MACOSX)) && !defined(MAR_NSS)| evaluates to true, meaning that DERFilePaths may not have undergone the memset [2] in step two above and is therefore uninitialized.

[1] http://mxr.mozilla.org/mozilla-central/source/modules/libmar/tool/mar.c#128
[2] http://mxr.mozilla.org/mozilla-central/source/modules/libmar/src/mar.h#23
[3] http://mxr.mozilla.org/mozilla-central/source/modules/libmar/tool/mar.c#140
[4] http://mxr.mozilla.org/mozilla-central/source/modules/libmar/tool/mar.c#364
[5] http://mxr.mozilla.org/mozilla-central/source/modules/libmar/tool/mar.c#292
(Assignee)

Comment 16

2 years ago
Created attachment 8698005 [details] [diff] [review]
Bug 1228332.diff
Attachment #8698003 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Almost there. Please don't break the commit message up into multiple lines even if it exceeds 80 characters. Having it on one line makes it easier to read on treeherder and the commit message isn't a code file, so doesn't have to abide by the 80 character rule.
Flags: needinfo?(bogdan.postelnicu)
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
Created attachment 8698010 [details] [diff] [review]
Bug 1228332.diff

Sure here it is, without line braking.
Attachment #8698005 - Attachment is obsolete: true
Flags: needinfo?(bogdan.postelnicu)
Comment on attachment 8698010 [details] [diff] [review]
Bug 1228332.diff

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

Thanks!
Attachment #8698010 - Flags: review+
Keywords: checkin-needed

Comment 20

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5bd67d576d7d
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5bd67d576d7d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.