1234206 - [CID 748764] modulo zero in cmscipher.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

If padsize in NSS_CMSCipherContext_Encrypt in cmscipher.c is 0 we modulo 0.

Comment 1

[kelbyludwig]

Attachment: mod-zero.patch

Comment 2

[kelbyludwig]

I was using this (simple) patch to test the waters with NSS patch submissions. Feedback is welcome!

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Hey Kelby, Bugzilla does unfortunately not seem to understand the patch format. Can you please check and export it with a few different options?

If you use Git it should be something like |git format-patch -kp -U8 -M| and for hg it's this in your ~/.hgrc:

[diff]
git = 1
show-function = 1
showfunc = 1
unified = 8

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I assigned the bug to you so everyone else knows you're working on it :)

Comment 5

[kelbyludwig]

Attachment: 0001-Bug-fix.patch

Thanks for the prompt response. 

New patch generated with:
git format-patch -kp -U8 -M -1 <commit hash>

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8706966 [details] [diff] [review]
0001-Bug-fix.patch

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

Thanks Kelby! That's a good start but needs some more fine-tuning. Looking forward to your next patch!

Oh, one more thing: please set the r? flag next time you're uploading a patch and obsolete the old version. That's unfortunately not automated...

::: cmscipher.c
@@ +691,5 @@
>  	if (pcount)
>  	    PORT_Memcpy (pbuf, input + ifraglen, pcount);
>      }
>  
> +	if (final) {

Nit: the indentation seems off here (probably just shouldn't touch that line)

@@ +693,5 @@
>      }
>  
> +	if (final) {
> +	if (padsize == 0) {
> +	    padlen = padsize

Missing a semicolon here. Also, if there's no padding then |padlen| should probably just be zero.

@@ +695,5 @@
> +	if (final) {
> +	if (padsize == 0) {
> +	    padlen = padsize
> +	} else {
> +	    padlen = padsize - (pcount % padsize);

I think we should move the |PORT_Memset()| call here, there is no point in calling it when padlen=0 as it would be no-op.

Comment 7

[kelbyludwig]

Attachment: 0001-Check-for-zero-length-padding-to-avoid-a-mod-by-zero.patch

Thanks for the feedback. Clearly, writing lots of Golang has affected my semicolon usage :)

Here is an updated patch that addresses your feedback. I followed the style-guide (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Developer_Tutorial) to verify correct usage of tabs/spaces. Let me know if there is an unacceptable difference.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8707093 [details] [diff] [review]
0001-Check-for-zero-length-padding-to-avoid-a-mod-by-zero.patch

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

::: lib/smime/cmscipher.c
@@ +697,5 @@
> +        padlen = 0;
> +    } else {
> +        padlen = padsize - (pcount % padsize);
> +        PORT_Memset (pbuf + pcount, padlen, padlen);
> +    }

Indentation is a little complicated... We usually try to stick with what the surrounding code does while we're working on clang-formatting everything. The code before used one <tab> (=8 spaces) so either please indent all of the above with tab or tab+4 spaces, or with 8/12 spaces.

The rest of the code looks great! I can give you r+ with the indentation fixes :)

Comment 9

[kelbyludwig]

Attachment: 0001-Check-for-zero-length-padding-to-avoid-a-mod-by-zero.patch

Switched to tab/tab+4 spacing.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8707126 [details] [diff] [review]
0001-Check-for-zero-length-padding-to-avoid-a-mod-by-zero.patch

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

Thanks Kelby! Now let's see if Martin agrees with me, I'm not actually an NSS peer so we need one more pair of eyes.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8707126 [details] [diff] [review]
0001-Check-for-zero-length-padding-to-avoid-a-mod-by-zero.patch

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

::: lib/smime/cmscipher.c
@@ +692,5 @@
>  	    PORT_Memcpy (pbuf, input + ifraglen, pcount);
>      }
>  
>      if (final) {
> +	if (padsize == 0) {

Let's make this <= 0.  pad_size isn't guaranteed to be non-negative, unfortunately.  We should probably have an assertion for that too.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #11)
> ::: lib/smime/cmscipher.c
> @@ +692,5 @@
> >  	    PORT_Memcpy (pbuf, input + ifraglen, pcount);
> >      }
> >  
> >      if (final) {
> > +	if (padsize == 0) {
> 
> Let's make this <= 0.  pad_size isn't guaranteed to be non-negative,
> unfortunately.  We should probably have an assertion for that too.

Kelby, can you please make this last change and then upload the final version of the patch? We will then get it checked in for you :)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Oh and while you're at it, please change the subject line of the patch. That's what we usually do:

Bug 1234206 - Check for zero length padding to avoid a mod by zero error r=mt,ttaubert

Comment 14

[kelbyludwig]

Attachment: 0001-Bug-1234206-Check-for-zero-length-padding-to-avoid-a.patch

Updated padsize check and updated subject. Thanks all.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8707437 [details] [diff] [review]
0001-Bug-1234206-Check-for-zero-length-padding-to-avoid-a.patch

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

Thank you!

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8707437 [details] [diff] [review]
0001-Bug-1234206-Check-for-zero-length-padding-to-avoid-a.patch

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

https://hg.mozilla.org/projects/nss/rev/9c91bc99ba4c