Closed Bug 1228332 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1340242)

Attachments

(1 file, 6 obsolete files)

The Static Analysis tool Coverity showed that a potentially bad access can result when dereferencing DERFilePaths.
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
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)
Whiteboard: CID 1340242
Component: Build Config → Application Update
Product: Core → Toolkit
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)
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-
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
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-
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
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
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
Flags: needinfo?(spohl.mozilla.bugs)
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)
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
Attachment #8697560 - Attachment is obsolete: true
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
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
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
Attached patch Bug 1228332.diff (obsolete) — Splinter Review
Attachment #8698003 - Attachment is obsolete: true
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
Attached patch Bug 1228332.diffSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/5bd67d576d7d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.