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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity showed that a potentially bad access can result when dereferencing DERFilePaths.

Comment 1

[Andi [:andi]]

Attachment: 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

Comment 2

[Gregory Szorc [:gps]]

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

I'm not an appropriate reviewer for this code. Redirecting.

Comment 3

[Stephen A Pohl [:spohl]]

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|.

Comment 4

[Andi [:andi]]

Attachment: 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.

Comment 5

[Stephen A Pohl [:spohl]]

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|.

Comment 6

[Andi [:andi]]

Attachment: Bug 1228332.diff

Comment 7

[Stephen A Pohl [:spohl]]

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!

Comment 8

[Stephen A Pohl [:spohl]]

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

Comment 9

[Andi [:andi]]

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

Comment 10

[Andi [:andi]]

By the way same guard should be done on certNames, since in case of failure this is printed as well.

Comment 11

[Stephen A Pohl [:spohl]]

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!

Comment 12

[Andi [:andi]]

Attachment: Bug 1228332.diff

Comment 13

[Stephen A Pohl [:spohl]]

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

Comment 14

[Andi [:andi]]

Attachment: Bug 1228332.diff

With description changed :)

Comment 15

[Stephen A Pohl [:spohl]]

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

Comment 16

[Andi [:andi]]

Attachment: Bug 1228332.diff

Comment 17

[Stephen A Pohl [:spohl]]

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.

Comment 18

[Andi [:andi]]

Attachment: Bug 1228332.diff

Sure here it is, without line braking.

Comment 19

[Stephen A Pohl [:spohl]]

Comment on attachment 8698010 [details] [diff] [review]
Bug 1228332.diff

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

Thanks!

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5bd67d576d7d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bd67d576d7d