1234510 - [CID 748763] modulo zero in p7local.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

sec_PKCS7Encrypt in p7local.c might do modulo zero (padsize).

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Avoid modulo zero in sec_PKCS7Encrypt

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8720609 [details] [diff] [review]
Avoid modulo zero in sec_PKCS7Encrypt

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

LGTM

Michelangelo wrote:
> to my knowledge a mod0 shoudln't
> happen over there, unless the compiler is "stupid enough" to not optmize
> the predicate.
that might be correct, but we probably shouldn't rely on it. And like this we can make coverity happy (and steve asked for something easy ;).

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8720609 [details] [diff] [review]
Avoid modulo zero in sec_PKCS7Encrypt

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

just noticed I messed up and gave you the wrong line...
You're right that the line I gave you in the e-mail is not a problem, but [1] is.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pkcs7/p7local.c#714

Comment 4

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Fix for mod0 in sec_PKCS7Encrypt

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8721087 [details] [diff] [review]
Fix for mod0 in sec_PKCS7Encrypt

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

LGTM

Comment 6

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8721087 [details] [diff] [review]
Fix for mod0 in sec_PKCS7Encrypt

https://hg.mozilla.org/projects/nss/rev/438462e134c1

Comment 7

[Michelangelo De Simone [Offline] [:mds]]

Nice, thank you Franziskus!

Comment 8

[Michelangelo De Simone [Offline] [:mds]]

Should we close this now?

Sorry, just asking out of ignorance: I'm not sure about the workflow in this case.:)

Comment 9

[Franziskus Kiefer [:franziskus]]

right, I marked it as resolved and set tracking for 3.23 (the next NSS release)

Comment 10

[Martin Thomson [:mt:]]

Attachment: bug1234510.patch

This is a better fix.  The current fix adds a build warning.

Comment 11

[Martin Thomson [:mt:]]

Attachment: bug1234510v2.patch

I'll let you pick which one of these looks best, I think that this might be better.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8728339 [details] [diff] [review]
bug1234510v2.patch

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

::: lib/pkcs7/p7local.c
@@ +710,5 @@
>  	    PORT_Memcpy (pbuf, input + ifraglen, pcount);
>      }
>  
>      if (final) {
> +	padlen = padsize - (pcount % padsize);

This would bring back mod zero, let's use the other patch.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/9059a7c7bc77

Comment 14

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Martin Thomson [:mt:] from comment #10)

> The current fix adds a build warning.

Do you have a dump of the warning?

Comment 15

[Franziskus Kiefer [:franziskus]]

(In reply to Michelangelo De Simone from comment #14)
> (In reply to Martin Thomson [:mt:] from comment #10)
> 
> > The current fix adds a build warning.
> 
> Do you have a dump of the warning?

See bug 1254406 comment 3.