313798 - AES is not supported in JSS

Reporter

Description

•

19 years ago

As a part of running SymKeyGen.java test, I tested AES.  This however seems to return DES instead.  There might be an issue with mapping table.  Hence the test fails with "wrong algorithm".

Testing AES
Received:DES algorithm
AES encryption failed
wrong algorithm
java.lang.Exception: wrong algorithm
        at org.mozilla.jss.tests.SymKeyGen.main(SymKeyGen.java:130)

Wan-Teh Chang

Comment 1

•

19 years ago

JSS supports AES.  Hopefully this is an error in
the test program.  Otherwise this would be a stop-ship
bug.

glen beasley

Assignee

Comment 2

•

19 years ago

Attached patch Part 1 of AES Fix (obsolete) — Details — Splinter Review

AES is broken in JSS. So far I fixed the deprecated code, but  
still have to fix the Mozilla-JSS provider. I will look into 
the Mozilla-JSS provider tomorrow.

Assignee: wtchang → glen.beasley

Status: NEW → ASSIGNED

Wan-Teh Chang

Comment 3

•

19 years ago

Comment on attachment 200941 [details] [diff] [review]
Part 1 of AES Fix 

In pkcs11/KeyType.java, we have:

    static public final KeyType
    AES       = new KeyType(new Algorithm[]
                            {
                            KeyWrapAlgorithm.AES_ECB,
                            KeyWrapAlgorithm.AES_CBC,
                            KeyWrapAlgorithm.AES_CBC_PAD,
                            EncryptionAlgorithm.AES_128_ECB,
                            EncryptionAlgorithm.AES_128_CBC,
                            EncryptionAlgorithm.AES_192_ECB,
                            EncryptionAlgorithm.AES_192_CBC,
                            EncryptionAlgorithm.AES_256_ECB,
                            EncryptionAlgorithm.AES_256_CBC,
                            EncryptionAlgorithm.AES_CBC_PAD,
                            },
                            "DES"
                        );

The string "DES" should be changed to "AES".

glen beasley

Assignee

Comment 4

•

19 years ago

Attached patch AES and RC2 fixes Mozilla-JSS — Details — Splinter Review

before this bug you could create an AES key for key sizes 128, 192, and 256 
but you could not confirm it's type once created. 

I added SymKeyGen.java and a new JCASymKeyGen.java to 
all.pl tests. 

I modified SymKeyGen to tests all keys JSS can create, 
and all EncryptionAlgorithms listed in EncryptionAlgorithm.java
with the exception of SHA1_HMAC which I will do in a separate
bug.

Most of the code in SymKeyGen is depredcated, so I created 
JCASymKeyGen.java this file has some noted "todo" due to 
bugs (which I will create separate bugs for), 
With this bug Mozilla-JSS will support the following: 
+            {"DES",  "DES/ECB/NoPadding", "DES/CBC/PKCS5Padding",  
+                     "DES/CBC/NoPadding" },
+            {"DESede", "DESede/ECB/NoPadding", "DESede/CBC/PKCS5Padding",  
+                     "DESede/CBC/NoPadding" },
+            {"AES", "AES/ECB/NoPadding",  "AES/CBC/NoPadding", 
+                     "AES/CBC/PKCS5Padding"}, 
+            {"RC2", "RC2/CBC/NoPadding", "RC2/CBC/PKCS5Padding"},

Attachment #200941 - Attachment is obsolete: true

Attachment #203316 - Flags: superreview?(wtchang)

Attachment #203316 - Flags: review?(Sandeep.Konchady)

Sandeep Konchady

Reporter

Comment 5

•

19 years ago

Comment on attachment 203316 [details] [diff] [review]
AES and RC2 fixes Mozilla-JSS

The changes look good.  I have two suggestions.

[1] JSSCipherSpi.java, 293-314: This block could be unified into one as there is nothing specific to IvParameterSpec or RC2ParameterSpec class.

        try {

           if( params instanceof IvParameterSpec ) {

               algParams = AlgorithmParameters.getInstance(algFamily);

               algParams.init(params);

           }

           if( params instanceof RC2ParameterSpec ) {

               algParams = AlgorithmParameters.getInstance(algFamily);
               algParams.init(params);

           }


[2] JCASymKeyGen.java: Would it be a good idea to System.exit(1) if the constructor is unable to initialize CryptoManager?

Attachment #203316 - Flags: review?(Sandeep.Konchady) → review+

Wan-Teh Chang

Comment 6

•

19 years ago

Comment on attachment 203316 [details] [diff] [review]
AES and RC2 fixes Mozilla-JSS

r=wtc.  I do have some questions and suggested changes.
Some of the suggested changes can be future work.

1.JSSProvider.java

We should review the entire file to see what is missing.
For example, for HMAC we only "put" HmacSHA1 right now,
even though JSS also supports HMAC with other message
digest algorithms.

2. crypto/EncryptionAlgorithm.java

>+            Mode m = Mode.NONE;
>+            try {
>+                m = (Mode) nameHash.get(name.toLowerCase());
>+                if ( m != null ) {
>+                    if ( m != Mode.NONE &&
>+                         m != Mode.ECB &&
>+                         m != Mode.CBC )
>+                        throw new NoSuchAlgorithmException(
>+                            "Unrecognized mode \"" + name + "\"");
>+                } else {
>+                    m = Mode.NONE;
>+                }
>+            } catch (NullPointerException npe) {
>+                m = Mode.NONE;
>             }

If you initialize m to Mode.NONE, then it seems that you
don't need to set m to Mode.NONE again in the else block
and the catch block.

If the nameHash.get() call returns null, why don't you
throw NoSuchAlgorithmException as the original code did?

>+            try {
>+                p = (Padding) nameHash.get(name.toLowerCase());
>+                if ( p != null ) {
>+                    if ( p != Padding.NONE &&
>+                         p != Padding.PKCS5 )
>+                        throw new NoSuchAlgorithmException("Unrecognized " +
>+                            "Padding type \"" + name + "\"");
>+                } else {
>+                    p = Padding.NONE; 
>+                }
>+            } catch (NullPointerException npe) {
>+                p = Padding.NONE;
>             }

Same comments as above.

>     public static final EncryptionAlgorithm
>+    AES_128_CBC_PAD = new EncryptionAlgorithm(CKM_AES_CBC_PAD, Alg.AES, Mode.CBC,
>+        Padding.PKCS5, IVParameterSpecClasses, 16, null, 128); // no oid

Why is it okay to omit the OID?  Does this algorithm not
have an OID?

3. crypto/SymmetricKey.java

>     public static final Type SHA1_HMAC = Type.SHA1_HMAC;
>+    public static final Type AES = Type.AES;

Should we also add SHA256_HMAC, etc.?

4. pkcs11/KeyType.java

>                             EncryptionAlgorithm.AES_256_ECB,
>                             EncryptionAlgorithm.AES_256_CBC,
>                             EncryptionAlgorithm.AES_CBC_PAD,
>+                            EncryptionAlgorithm.AES_128_CBC_PAD,
>+                            EncryptionAlgorithm.AES_192_CBC_PAD
>                             },

If changing the order of the array elements does not break
backward compatibility, we should replace AES_CBC_PAD by
AES_256_CBC_PAD and put it after AES_192_CBC_PAD.

5. pkcs11/PK11SymKey.java

>+        } else if(kt == KeyType.AES) {
>+            return AES;
>         } else {
>             Assert.notReached("Unrecognized key type");
>             return DES;

Should we also handle KeyType.SHA1_HMAC?

6. provider/java/security/IvAlgorithmParameters.java

>         if( clazz != null && !(clazz.isInstance(ivParamSpec)) ) {
>             Class paramSpecClass = ivParamSpec.getClass();
>             throw new InvalidParameterSpecException(
>-                "Parameter spec has class " + paramSpecClass.getName());
>+                "IV getParameterSpec has class " + paramSpecClass.getName());
>         }

Should the new error message say "IvParameterSpec has class "?
"IV getParameterSpec" sounds strange.

7/ provider/javax/crypto/JSSCipherSpi.java

>+    private int keySize;

It may be a good idea to rename this keyStrength and
note that it is only used for RC2.

>+            if( paramClasses[i].equals( javax.crypto.spec.IvParameterSpec.class ) ) {
>+                algParSpec = new javax.crypto.spec.IvParameterSpec(iv);
>+                break;
>+            }
>+            if( paramClasses[i].equals( RC2ParameterSpec.class ) ) {
>+                algParSpec = new RC2ParameterSpec(keySize, iv);  
>+                break;
>+            }

Use "else if" for the second "if".

>+            if( params instanceof IvParameterSpec ) {
>+                algParams = AlgorithmParameters.getInstance(algFamily);
>+                algParams.init(params);
>+            }
>+            if( params instanceof RC2ParameterSpec ) {
>+                algParams = AlgorithmParameters.getInstance(algFamily); 
>+                algParams.init(params);
>+            }

Use "else if" for the second "if".

Why don't you pass "Mozilla-JSS" as the second argument
to the two AlgorithmParameters.getInstance calls?  The
original code did.

Attachment #203316 - Flags: superreview?(wtchang) → superreview+

Wan-Teh Chang

Comment 7

•

19 years ago

Comment on attachment 203316 [details] [diff] [review]
AES and RC2 fixes Mozilla-JSS

I forgot to note that I didn't have time to review
your changes to the tests in this patch.  Sorry.

glen beasley

Assignee

Comment 8

•

19 years ago

(In reply to comment #6)
> (From update of attachment 203316 [details] [diff] [review] [edit])
> r=wtc.  I do have some questions and suggested changes.
> Some of the suggested changes can be future work.
> 
> 1.JSSProvider.java
> 
> We should review the entire file to see what is missing.
> For example, for HMAC we only "put" HmacSHA1 right now,
> even though JSS also supports HMAC with other message
> digest algorithms.

agreed. I will makes some bugs.  
> 
> 2. crypto/EncryptionAlgorithm.java
> 
> >+            Mode m = Mode.NONE;
> >+            try {
> >+                m = (Mode) nameHash.get(name.toLowerCase());
> >+                if ( m != null ) {
> >+                    if ( m != Mode.NONE &&
> >+                         m != Mode.ECB &&
> >+                         m != Mode.CBC )
> >+                        throw new NoSuchAlgorithmException(
> >+                            "Unrecognized mode \"" + name + "\"");
> >+                } else {
> >+                    m = Mode.NONE;
> >+                }
> >+            } catch (NullPointerException npe) {
> >+                m = Mode.NONE;
> >             }
> 
> If you initialize m to Mode.NONE, then it seems that you
> don't need to set m to Mode.NONE again in the else block
> and the catch block.
> 
> If the nameHash.get() call returns null, why don't you
> throw NoSuchAlgorithmException as the original code did?
> 
> >+            try {
> >+                p = (Padding) nameHash.get(name.toLowerCase());
> >+                if ( p != null ) {
> >+                    if ( p != Padding.NONE &&
> >+                         p != Padding.PKCS5 )
> >+                        throw new NoSuchAlgorithmException("Unrecognized " +
> >+                            "Padding type \"" + name + "\"");
> >+                } else {
> >+                    p = Padding.NONE; 
> >+                }
> >+            } catch (NullPointerException npe) {
> >+                p = Padding.NONE;
> >             }
> 
> Same comments as above.

I removed this code it is from bug 314266 and is not needed 
for this fix plus it needs more work. I will update 314266. 

> 
> >     public static final EncryptionAlgorithm
> >+    AES_128_CBC_PAD = new EncryptionAlgorithm(CKM_AES_CBC_PAD, Alg.AES, Mode.CBC,
> >+        Padding.PKCS5, IVParameterSpecClasses, 16, null, 128); // no oid
> 
> Why is it okay to omit the OID?  Does this algorithm not
> have an OID?
I could not find an OID. 
> 
> 3. crypto/SymmetricKey.java
> 
> >     public static final Type SHA1_HMAC = Type.SHA1_HMAC;
> >+    public static final Type AES = Type.AES;
> 
> Should we also add SHA256_HMAC, etc.?

yes, created bug 317045

> 
> 4. pkcs11/KeyType.java
> 
> >                             EncryptionAlgorithm.AES_256_ECB,
> >                             EncryptionAlgorithm.AES_256_CBC,
> >                             EncryptionAlgorithm.AES_CBC_PAD,
> >+                            EncryptionAlgorithm.AES_128_CBC_PAD,
> >+                            EncryptionAlgorithm.AES_192_CBC_PAD
> >                             },
> 
> If changing the order of the array elements does not break
> backward compatibility, we should replace AES_CBC_PAD by
> AES_256_CBC_PAD and put it after AES_192_CBC_PAD.
added a comment but I left AES_CBC_PAD in the same order 
due to backward compatiblity.  
> 
> 5. pkcs11/PK11SymKey.java
> 
> >+        } else if(kt == KeyType.AES) {
> >+            return AES;
> >         } else {
> >             Assert.notReached("Unrecognized key type");
> >             return DES;
> 
> Should we also handle KeyType.SHA1_HMAC?

> 
> 6. provider/java/security/IvAlgorithmParameters.java
> 
> >         if( clazz != null && !(clazz.isInstance(ivParamSpec)) ) {
> >             Class paramSpecClass = ivParamSpec.getClass();
> >             throw new InvalidParameterSpecException(
> >-                "Parameter spec has class " + paramSpecClass.getName());
> >+                "IV getParameterSpec has class " + paramSpecClass.getName());
> >         }
> 
> Should the new error message say "IvParameterSpec has class "?
> "IV getParameterSpec" sounds strange.
> 
> 7/ provider/javax/crypto/JSSCipherSpi.java
> 
> >+    private int keySize;
> 
> It may be a good idea to rename this keyStrength and
> note that it is only used for RC2.
okay
> 
> >+            if( paramClasses[i].equals( javax.crypto.spec.IvParameterSpec.class ) ) {
> >+                algParSpec = new javax.crypto.spec.IvParameterSpec(iv);
> >+                break;
> >+            }
> >+            if( paramClasses[i].equals( RC2ParameterSpec.class ) ) {
> >+                algParSpec = new RC2ParameterSpec(keySize, iv);  
> >+                break;
> >+            }
> 
> Use "else if" for the second "if".
> 
> >+            if( params instanceof IvParameterSpec ) {
> >+                algParams = AlgorithmParameters.getInstance(algFamily);
> >+                algParams.init(params);
> >+            }
> >+            if( params instanceof RC2ParameterSpec ) {
> >+                algParams = AlgorithmParameters.getInstance(algFamily); 
> >+                algParams.init(params);
> >+            }
> 
> Use "else if" for the second "if".
okay.
> 
> Why don't you pass "Mozilla-JSS" as the second argument
> to the two AlgorithmParameters.getInstance calls?  The
> original code did.
> 
because there is no reason in the CipherSpi to not use
the JCE AlgorithmParameters implementation, and for Mozilla-JSS to be 
useful getParameters().getEncoded() needs to work correctly,
the choice was either to implement getEncoded for Mozilla-JSS's
IvParameterSpec and RC2ParameterSpec or use the JCE default 
implementation. 

the JCASymKeyGen does the following:
creates Algorithm key then for each cipher that the Algorithm supports
loops and tests:
tests encrypt/decrypt with Mozilla-JSS
tests encrypt Mozilla-JSS decrypt with otherProvider(SunJCE)
test decrypt other Provider / encrypt Mozilla-JSS
in the cipher test after the encrypt the code encode the parameters 
then decodes the parameters to decrypt to simulate a more realistic
usage. Before this bug it was not possible to encode the parameters.

Wan-Teh Chang

Comment 9

•

19 years ago

Glen, in pkcs11/KeyType.java can we use AES_256_CBC_PAD
instead of AES_CBC_PAD?  AES_CBC_PAD is badly named.
Would this break backward compatibility?

glen beasley

Assignee

Comment 10

•

19 years ago

(In reply to comment #9)
> Glen, in pkcs11/KeyType.java can we use AES_256_CBC_PAD
> instead of AES_CBC_PAD?  AES_CBC_PAD is badly named.
> Would this break backward compatibility?
> 
it would only break compatibility if a customer really 
coded poorly and was trying to something very odd. 
There is already a warning in KeyType.java

 * Although the KeyType class is public, it should
 * be considered private. We made the KeyType class
 * public so that we can force it to load during
 * CryptoManager.initialize(), before we install JSS
 * as a provider.

I checked all usages in JSS of the KeyType Class and 
the existing code is basically the form: 
if( ((PK11SymKey)key).getKeyType() !=
                    KeyType.getKeyTypeFromAlgorithm(algorithm) ) 

therefore I changed to AES_256_CBC_PAD.

glen beasley

Assignee

Comment 11

•

19 years ago

Mozilla-JSS CipherSpi needs more work there are existings bugs:

PBE cipher algorithms bug 195128
RC2 works for {"RC2", "RC2/CBC/NoPadding", "RC2/CBC/PKCS5Padding"} but bug 205070 should be checked. 
RC4 - bug 313778
HMACSHA256 -  bug 317045

This bugs adds SymkeyGen.java and JCASymkeyGen.java to all.pl. 

Checking in org/mozilla/jss/JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.20; previous revision: 1.19
done
Checking in org/mozilla/jss/crypto/EncryptionAlgorithm.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/crypto/EncryptionAlgorithm.java,v  <--  EncryptionAlgorithm.java
new revision: 1.11; previous revision: 1.10
done
Checking in org/mozilla/jss/crypto/SymmetricKey.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/crypto/SymmetricKey.java,v  <--  SymmetricKey.java
new revision: 1.11; previous revision: 1.10
done
Checking in org/mozilla/jss/pkcs11/KeyType.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/pkcs11/KeyType.java,v  <--  KeyType.java
new revision: 1.10; previous revision: 1.9
done
Checking in org/mozilla/jss/pkcs11/PK11SymKey.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/pkcs11/PK11SymKey.java,v  <--  PK11SymKey.java
new revision: 1.8; previous revision: 1.7
done
Checking in org/mozilla/jss/provider/java/security/IvAlgorithmParameters.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/provider/java/security/IvAlgorithmParameters.java,v  <--  IvAlgorithmParameters.java
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/security/jss/org/mozilla/jss/provider/java/security/RC2AlgorithmParameters.java,v
done
Checking in org/mozilla/jss/provider/java/security/RC2AlgorithmParameters.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/provider/java/security/RC2AlgorithmParameters.java,v  <--  RC2AlgorithmParameters.java
initial revision: 1.1
done
Checking in org/mozilla/jss/provider/javax/crypto/JSSCipherSpi.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/provider/javax/crypto/JSSCipherSpi.java,v  <--  JSSCipherSpi.java
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/security/jss/org/mozilla/jss/tests/JCASymKeyGen.java,v
done
Checking in org/mozilla/jss/tests/JCASymKeyGen.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/tests/JCASymKeyGen.java,v  <--  JCASymKeyGen.java
initial revision: 1.1
done
Checking in org/mozilla/jss/tests/SymKeyGen.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/tests/SymKeyGen.java,v  <--  SymKeyGen.java
new revision: 1.7; previous revision: 1.6
done
Checking in org/mozilla/jss/tests/all.pl;
/cvsroot/mozilla/security/jss/org/mozilla/jss/tests/all.pl,v  <--  all.pl
new revision: 1.29; previous revision: 1.28
done

Status: ASSIGNED → RESOLVED

Closed: 19 years ago

Resolution: --- → FIXED

Wan-Teh Chang

Updated

•

19 years ago

Target Milestone: --- → 4.2

Part 1 of AES Fix 19 years ago glen beasley 2.54 KB, patch		Details \| Diff \| Splinter Review
AES and RC2 fixes Mozilla-JSS 19 years ago glen beasley 54.14 KB, patch	Sandeep.Konchady : review+ wtc : superreview+	Details \| Diff \| Splinter Review

Bugzilla

Quick Search

AES is not supported in JSS

Categories

(JSS Graveyard :: Library, defect)

Tracking

(Not tracked)

People

(Reporter: Sandeep.Konchady, Assigned: glenbeasley)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file, 1 obsolete file)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Updated

Attachment

General

Description

File Name

Content Type