404286 - ShortBufferException in Mozilla-JSS CipherSpi

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect, P1)

Description

[glen beasley]

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)

Comment 1

[glen beasley]

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

Comment 2

[glen beasley]

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

Comment 3

[Wan-Teh Chang]

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?

Comment 4

[glen beasley]

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

[Wan-Teh Chang]

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

Comment 6

[Wan-Teh Chang]

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

[Kai Engert (:KaiE:)]

(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"

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 9

[glen beasley]

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)
     */

Comment 10

[glen beasley]

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

Comment 11

[Wan-Teh Chang]

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;

Comment 12

[glen beasley]

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.