This week's book giveaway is in the Design forum.
We're giving away four copies of Design for the Mind and have Victor S. Yocco on-line!
See this thread for details.
Win a copy of Design for the Mind this week in the Design forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

AES Encryption Service

 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm working on an AES Encryption service. I was wondering in you wouldn't mind taking a look at the design and look for any security holes:

 
James Sabre
Ranch Hand
Posts: 781
Java Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator


1) Why are you using PBE rather than loading the key from some form of key store? It is certainly insecure to hold the password in the source code.
2) Any exception in either encryption or decryption is fatal so you cannot continue. Either the exception must be passed back to the caller or you should wrap the exception in an Error or your own exception and throw that.
3) In the decryption, if the split() does not create three sections then you cannot continue so you should throw an exception. I would follow the same approach as in (2).
4) Since you use utf-8 when converting the cleartext String to bytes you should use utf-8 when converting the decrypted bytes to a String.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'll definitely be moving the password to a keystore as we need different keys for each environment (dev, staging, production). This was more illustrative of what i was doing in one chunk of code. Mostly I wanted to confirm that there were no serious flaws in the encryption/decryption routine.

I will definitely take your advice and add throws rather than catching those exceptions.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Here's the updated version with the throws suggestion implemented and the character encoding fixed:

 
James Sabre
Ranch Hand
Posts: 781
Java Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I still don't like the fact that you are using PBE. If you are going to use some form of key store then it is not needed.

The users of your methods don't care about the exceptions that are thrown. What can they do if they get a NoSuchAlgorithmException or any of the other exceptions you throw? Nothing since they are ALL fatal and would represent a system error. In your position I would catch any exception and wrap it in an exception I derived from RuntimeException and throw that.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I understand that from the perspective of not checking in the key etc. that a keystore is more secure, but is there a technical security issue with PBE?

 
James Sabre
Ranch Hand
Posts: 781
Java Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
David Sawyer wrote:I understand that from the perspective of not checking in the key etc. that a keystore is more secure, but is there a technical security issue with PBE?


PBE is meant for use where a user provides the password that only he knows. Your code does not allow a user to do do this.

I still don't like your exception handling since you are not actually wrapping the exception; you just wrap the message. My encryption exception is


and I would use it as


so that all the information available in the wrapped exception is retained.

P.S. There is no point in generating a 256 bit key from your password if the entropy of your password is less than 256 bits and almost nobody will be able to remember a 256 bit (32 byte) password or type it in without error. Note, to get an entropy of 256 requires about 42 ASCII printable characters.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
James, thank you so much for the time. Here's what I've come up with:

First a script to generate the keystore/key:


Next the revised AESEncryptionException:


The AESEncryptionService Interface:


The AESEncryptionServiceImpl:


And Unit Test to make sure it all works:

 
James Sabre
Ranch Hand
Posts: 781
Java Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Much much better David. The only significant change I would make is that I would pass the key (as a SecretKey object) into the AESEncryptionServiceImpl when it was constructed so that the encryption methods were isolated from the key storage method (separation of concerns). In your code there are some 'tweeks' I would make

1) I would make the getKeyFromStore() method return a SecretKey (separation of concerns again) then the code
would not be needed. When you generate the key store you specified the key as being AES so you would not need to specify or convert it to AES when you read it from the key store.
2) In your decryption method I would either catch the AESEncryptionException before the Exception and then just re-throw it since there is no point in re-wrapping the wrapped exception OR I would put the exception handling code only round the actual decryption code and not to include the code that is involved in splitting the IV from the ciphertext.
3) You don't handle the exceptions in generateKeyStore() and getKeyFromStore() in the same way as you do elsewhere.

I like the fact that you have test code.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nice. I like that much better. Here's the current setup:

Interface:


Impl:


Factory:


Script to generate keystore/key:


Unit Test:
 
Rob Spoor
Sheriff
Pie
Posts: 20511
54
Chrome Eclipse IDE Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
David, I cut up the throws-clauses of your encrypt and decrypt messages in your second post a bit so that the general layout of this thread isn't so messed up anymore. Can you watch it a bit next time? Thanks.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Rob Prime wrote:Can you watch it a bit next time? Thanks.
Definitely. Will do.
 
David Sawyer
Greenhorn
Posts: 14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Did anyone else have any security concerns with this final implementation or does this seem secure to everyone?
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic