ShortBufferException in Mozilla-JSS CipherSpi

RESOLVED FIXED in 4.2.6

Status

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: glenbeasley, Assigned: glenbeasley)

Tracking

(Blocks: 1 bug)

4.2.5
4.2.6
x86
All

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Reported internally at Sun by Olaf Hey:

Caused by: javax.crypto.ShortBufferException: 32 needed, 20480 supplied
         at
org.mozilla.jss.provider.javax.crypto.JSSCipherSpi.engineUpdate(JSSCipherSpi.java:338)

         at javax.crypto.Cipher.update(DashoA12275)
         at com.jcraft.jsch.jce.TripleDESCBC.update(TripleDESCBC.java:81)
(Assignee)

Updated

11 years ago
Priority: -- → P1
(Assignee)

Comment 1

11 years ago
Created attachment 289259 [details] [diff] [review]
buffercheck was incorrect

It appears we never tested JSSCipherSpi.engineUpdate since it's been implemented. The change to JSSCipherSpi.engineGetOutputSize is to have the same results as the SunJCE.
Attachment #289259 - Flags: review?(wtc)
(Assignee)

Comment 2

11 years ago
java -cp ./jss4.jar org/mozilla/jss/tests/JCASymKeyGen .
main: jss library loaded
SunJCE: SunJCE Provider (implements RSA, DES, Triple DES, AES, Blowfish, ARCFOUR, RC2, PBE, Diffie-Hellman, HMAC)
Mozilla-JSS: Provides Signature, Message Digesting, and RNG
Key DES generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested DES/ECB/NoPadding
Mozilla-JSS and  SunJCE tested DES/CBC/PKCS5Padding
Mozilla-JSS and  SunJCE tested DES/CBC/NoPadding
Key DESede generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested DESede/ECB/NoPadding
Mozilla-JSS and  SunJCE tested DESede/CBC/PKCS5Padding
Mozilla-JSS and  SunJCE tested DESede/CBC/NoPadding
Key AES generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested AES/ECB/NoPadding
Mozilla-JSS and  SunJCE tested AES/CBC/NoPadding
Mozilla-JSS and  SunJCE tested AES/CBC/PKCS5Padding
Key RC2 generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested RC2/CBC/NoPadding
Mozilla-JSS and  SunJCE tested RC2/CBC/PKCS5Padding
Key PBEWithMD5AndDES generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested DES/ECB/NoPadding
Key PBEWithSHA1AndDES generation done by Mozilla-JSS version 4.2.5
Key PBEWithSHA1AndDESede generation done by Mozilla-JSS version 4.2.5
Mozilla-JSS and  SunJCE tested DESede/ECB/NoPadding
OS: Windows Vista → All

Comment 3

11 years ago
Comment on attachment 289259 [details] [diff] [review]
buffercheck was incorrect

Hi Glen, I don't understand the change
to engineGetOutputSize.  Could you explain?
The original code returns
(ceiling(inputLen/blockSize) + 1) * blockSize.
The new code removes the extra blockSize only
for the case inputLen is a multiple of blockSize.
Why?
(Assignee)

Comment 4

11 years ago
per comment #3 In my testing I noticed that SunJCE had different results when calculating the outputsize then Mozilla-JSS. It was clear that the SunJCE checks if the inputLen is multiple of blocksize and even if PCS5Padding is specifed, padding will not be used so the actual outputlen needed is the same as the inputLen. The change to engingeGetOutputSize is just to have the same behavior as the SunJCE, although it is an unnecessary change and if you think it is not needed then I don't mind not checking it in. 

Comment 5

11 years ago
Glen, do we match SunJCE's output size when inputLen is *not*
a multiple of blockSize?

Comment 6

11 years ago
Comment on attachment 289259 [details] [diff] [review]
buffercheck was incorrect

>+            //ensure the recovered bytes equals the orginal plaintext
>+            boolean compare = true;

"equal" sounds better than "compare".  Is "equal" a Java keyword?

Comment 7

11 years ago
(In reply to comment #6)
> (From update of attachment 289259 [details] [diff] [review])
> >+            //ensure the recovered bytes equals the orginal plaintext
> >+            boolean compare = true;
> 
> "equal" sounds better than "compare".  Is "equal" a Java keyword?


I don't know the answer, but I usually prefix names of boolean variables with "is", so you could name it "is_equal" or "isEqual"
> SunJCE checks if the inputLen is multiple of blocksize and even if 
> [PKCS5 Padding] is specifed, padding will not be used so the actual 
> outputlen needed is the same as the inputLen.

Isn't that behavior a bug?
(Assignee)

Comment 9

11 years ago
Comment on attachment 289259 [details] [diff] [review]
buffercheck was incorrect

I was incorrect in my understanding of engineGetOutputSize. When I noticed Mozilla-JSS and SunJCE calculated different sizes I made some quick incorrect assumptions.  

If there is no padding then SunJCE will return the totallen which is the current given inputlen + uprocessed bufferred data from a previous call.

I will open a separate bug bug for Mozilla-JSS engineGetOutputSize. 

SunJCE javadoc for getOutputSize

    /**
     * Returns the length in bytes that an output buffer would need to be in
     * order to hold the result of the next <code>update</code> or
     * <code>doFinal</code> operation, given the input length
     * <code>inputLen</code> (in bytes).
     *
     * <p>This call takes into account any unprocessed (buffered) data from a
     * previous <code>update</code> call, and padding.
     *
     * <p>The actual output length of the next <code>update</code> or
     * <code>doFinal</code> call may be smaller than the length returned by
     * this method.
     *
     * @param inputLen the input length (in bytes)
     *
     * @return the required output buffer size (in bytes)
     */
Attachment #289259 - Flags: review?(wtc)
(Assignee)

Comment 10

11 years ago
Created attachment 293901 [details] [diff] [review]
shortbuffer exception and multipart iperation fix

Mozilla-JSS provider now supports Multipart encryption/decryption. 
Mozilla-JSS agrees with the Sun-JCE on outputsize at least on 
the limited testing I did. extensive testing should be done at a later date.
Attachment #289259 - Attachment is obsolete: true
Attachment #293901 - Flags: review?(wtc)
(Assignee)

Updated

11 years ago
Attachment #293901 - Flags: review?(wtc)
(Assignee)

Updated

11 years ago
Blocks: 413015

Comment 11

11 years ago
Comment on attachment 293901 [details] [diff] [review]
shortbuffer exception and multipart iperation fix

Glen, I only reviewed your changes to provider/javax/crypto/JSSCipherSpi.java.

>+        int total = buffered + inputLen;
[...]       
>+        if (encAlg.isPadded()) {
>+            total = buffered + (blockSize-1) + inputLen;

You can use += here:

              total += blockSize-1;

>     public int engineUpdate(byte[] input, int inputOffset, int inputLen,
>         byte[] output, int outputOffset) throws ShortBufferException
[...]
>+        buffered += bytes.length;

As we discussed last week, I believe this line is wrong and should be reduced
modulo blockSize:

          buffered += bytes.length;
          buffered %= blockSize;
(Assignee)

Comment 12

11 years ago
Wan-Teh for JSSCipherSpi.java I only check in the changes you 
approved. 

cvs diff -r 1.7 -r 1.6 JSSCipherSpi.java 
Index: JSSCipherSpi.java
===================================================================
RCS file: /cvsroot/mozilla/security/jss/org/mozilla/jss/provider/javax/crypto/JSSCipherSpi.java,v
retrieving revision 1.7
retrieving revision 1.6
diff -r1.7 -r1.6
366c366
<         if( bytes.length > output.length-outputOffset ) {
---
>         if( bytes.length < output.length-outputOffset ) {
407c407
<         if( bytes.length > output.length-outputOffset ) {
---
>         if( bytes.length < output.length-outputOffset ) {

I created bug 413015. my plan is to close this bug and make  further changes in 
413015. 
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.