[Esapi-dev] [OWASP-ESAPI] Encryptor.encrypt/decrypt(String) deprecated?

[Jeff Williams]

Hi,

I haven't looked into this much, so forgive me for just basing these
comments on the tiny code snippets in the email.

I'm afraid I still don't understand why we can't hide all the PlainText and
CipherText inside the encrypt(string) and decrypt(string) methods.  I mean,
couldn't we have...(pseudocode alert)

String encrypt( String input ) {
	PlainText p = new PlainText( input );
	CipherText c = Encryptor.encrypt( p );
	return Base64.encode( c.writeObject() );  // serialized CipherText
}

String decrypt( String input ) {
	CipherText c = Base64.decode( CipherText.readObject(input) );  //
deserialize
	PlainText p = Encryptor.decrypt( c );
	Return p.toString();
}


> ...make your Encryptor.MasterKey to be 256-bits long to begin with should
> you decide to ever do this. A 256-bit AES key can be used with 128-bit
> AES encryption; it just uses the first 128-bits.)

Why don't we just do this always?  I don't see much downside here.  Does it
require the extra Sun strong policy files?

--Jeff

Jeff Williams, Chair
The OWASP Foundation
work: 410-707-1487
main: 301-604-4882



-----Original Message-----
From: Kevin W. Wall [mailto:kevin.w.wall at gmail.com] 
Sent: Wednesday, December 16, 2009 8:14 PM
To: Josh Drummond; ESAPI-Developers
Cc: Jim Manico; Jeff Williams
Subject: Re: [OWASP-ESAPI] Encryptor.encrypt/decrypt(String) deprecated?

Hi Josh,

I realize it's been a while this whole issue has been brought up, but I've
finally gotten some vacation time to think about this and respond to it.

I'm trying to think of some alternatives to what you laid out as
(full thread provided below for the context of others):

> // encrypt is fairly easy, creating a PlainText object from a String
> public void saveData(String data)
> {
>         CipherText ciphertext =
>             ESAPI.encryptor().encrypt( new PlainText(data) );
>         // insert into some database table as a varchar
>         database.store(ciphertext.getEncodedIVCipherText());
> }
>
> // decrypt, or more precisely creating a CipherText object from a String
takes
more work
> public String getData()
> {
>      // retrieve varchar data from database
>     String cipherTextString = database.retrieve();
>
>     DefaultCipherText cipherText = new DefaultCipherText()
>
>     // *** uh oh, many lines of code here to convert string to CipherText
object
>     // basically implementing similar code as
JavaEncryptor.decrypt(String)
each time
>     // where in a simple case isn't greatly needed
>
>     PlainText recoveredPlaintext = ESAPI.encryptor().decrypt(cipherText)
);
>     return recoveredPlaintext.toString();
> }

in the context of storing and retrieving encrypted data to/from a DB.

Specifically, I'm trying to come up with an alternative of writing that
"many lines of code here to convert string to CipherText object" that you
referred to in the getData() method, as I agree, that is way too
complicated.

I guess my own thinking along these lines was only from a greenfield
perspective. Rather than storing CipherText.getEncodedIVCipherText()
I was anticipating that such a scenario as you laid out with storing /
retrieving encrypted data from a DB would be done by storing the complete
CipherText object itself. That is one reason that I made it Serializable.
Unfortunately, I didn't go much beyond that to support serializing the
object.

I could rather easily add support for the CipherText serialization.
Something

  public byte[] asSerializedObject() throws IOException
and
  public static CipherText createFromSerializedObject(byte[]
serializedObject)

(The latter would normally just be a CTOR, but since CipherText is an
interface
[which may not make sense, see DISCUSS comment in code], a static factory
method
would have to do.)

This would lead to something like this:

// encrypt and store
public void saveData(String data) throws IOException
{
        CipherText ciphertext =
            ESAPI.encryptor().encrypt( new PlainText(data) );
        byte[] serializedCipherText = ciphertext.asSerializedObject();
        // insert into some database table as a blob
        database.store( serializedCipherText );
        // OR... base64 encode and store as a varchar
        // database.store(
        //     ESAPI.encoder().encodeForBase64(serializedCipherText, false)
);
}

// retrieve encrypted data and decrypt
public String getData(String primaryDBkey)
{
     // retrieve blob data from database
    byte[] serializedCipherText = database.retrieve(primaryDBkey);
    CipherText ct =
        CipherText.createFromSerializedObject(serializedCipherText);
    //  OR...if stored in DB as base64 encoded varchar...
    // CipherText ct =
    //   CipherText.createFromSerializedObject(
    //     ESAPI.encoder.decodeFromBase64(
    //                                database.retrieve(primaryDBkey) ) );
    //

    PlainText recoveredPlaintext = ESAPI.encryptor().decrypt(ct) );
    return recoveredPlaintext.toString();
}

The major downside to this of course is the size of serialized CipherText
object is going to be larger than simply storing the base64 encoded IV +
raw ciphertext bytes. If you already have your DB storage space allocated
I suppose this could be a showstopper. However, there are advantages of
storing a serialized CipherText object too. For instance, if today you
decided to store data encrypted using 128-bit AES and later you need
to switch to 256-bit AES (say for compliance with some future regulatory
issues, etc.) there would not be any code changes needed nor would you
have to store additional data to distinguish what data was encrypted
in which manner. (Note: You should use the Encryptor encrypt() /
decrypt() methods that pass an explicity SecretKey parameter or make
your Encryptor.MasterKey to be 256-bits long to begin with should
you decide to ever do this. A 256-bit AES key can be used with 128-bit
AES encryption; it just uses the first 128-bits.)

The only alternative to this that I see is to remove the deprecation on
the Encryptor.encrypt(String) and Encryptor.decrypt(String) interfaces.
(I would leave it on the LegacyEncryptor class though as that should
always be avoided.)

If this deprecation be is removed, I would have to insert some lengthy
caveat into these methods in the Encryptor interface describing why they
are dangerous and should in general be avoided lest people use them when
authenticity really *is* an issue. And that's one discussion I was hoping
to avoid, in part because I think most developers will skip over those
details and use them when inappropriate regardless of warnings and in part
because really understanding why this is an issue requires a level of
understanding why beyond what even most information security professionals
comprehend about cryptographic attacks. To a large part, I find that people
tend to dismiss warnings that they don't understand.

Anyhow, would like to here your view on where storing serialized CipherText
objects rather than just the shorter base64-encoded IV + raw ciphertext
bytes in a DB might be an acceptable alternative to this. If not, the
deprecation for Encryptor.encrypt(String) and Encryptor.decrypt(String)
may need to be replaced with a caveat that no one will ever read, much
less heed.

Jim, Jeff: Please pipe in here with your thoughts on this too.

Thanks,
-kevin
=========================================
On 2009/11/12, Josh Drummond wrote:
> Hi Kevin,
> 
> Thanks for the link, I've read parts of that before in other documentation
from SVN, but this is a more comprehensive document.  I'm obviously pretty
new to the ESAPI project, so I don't have the "baggage" of 1.4 with me and
the improvements in 2.0 look great (i.e. ECB->CBC, etc).  Instead I'm
looking at it to eventually replace an  in-house wrapper around JCE my
organization is using.  Also in our use cases, a web application encrypts
the data when stored at rest in a database, and decrypts before displaying
to the user or transmitting somewhere, so we don't have the case of
transmitted data in its encrypted form with others outside the web
application, so that's kind of where I'm coming from when saying an
interface for "simpler cases" without as much a need for authenticity and
integrity, assuming the web application is the only actor
encrypting/decrypting data.
> 
> For example, here is an example in the document you referenced:
> 
>         CipherText ciphertext =
>             ESAPI.encryptor().encrypt( new PlainText(myplaintext) );
>         PlainText recoveredPlaintext =
ESAPI.encryptor().decrypt(ciphertext) );
>         assert myplaintext.equals( recoveredPlaintext.toString() );
> 
> But in reality it isn't quite as simple as that, most often the encrypt()
and decrypt() would be invoked from different calls/methods, that have to
return the result as a string such that it is in a form the user can read,
or some system can transmit or process.
> 
> For example, two possible calls in a web application that gets data from a
user, persists its data to a database, and displays it back:
> 
> // encrypt is fairly easy, creating a PlainText object from a String
> public void saveData(String data)
> {
>         CipherText ciphertext =
>             ESAPI.encryptor().encrypt( new PlainText(data) );
>         // insert into some database table as a varchar
>         database.store(ciphertext.getEncodedIVCipherText());
> }
> 
> // decrypt, or more precisely creating a CipherText object from a String
takes more work
> public String getData()
> {
>      // retrieve varchar data from database
>     String cipherTextString = database.retrieve();
>     
>     DefaultCipherText cipherText = new DefaultCipherText()
> 
>     // *** uh oh, many lines of code here to convert string to CipherText
object
>     // basically implementing similar code as
JavaEncryptor.decrypt(String) each time
>     // where in a simple case isn't greatly needed
> 
>     PlainText recoveredPlaintext = ESAPI.encryptor().decrypt(cipherText)
);
>     return recoveredPlaintext.toString();
> }
> 
> 
> 
> I suppose that was my biggest concern/curiosity when inquiring why the two
simpler String only methods were deprecated.  Jim's idea would work too, at
least to the API user you still have the option to deal directly with
Strings/simpler data types when the extra features aren't needed as much,
but can specify additional parameters rather than just using the Master
values.
> 
> I had a couple other questions about the encryption module, I thought
about submitting issues but am a little hesitant until I learn the
design/intent of the architecture better first :)
> - Feature? A random generator utility for an IV, similar to the one for
master key and salt, would be useful for those cases where one must use a
fixed IV and need to create one from scratch.
> - Why is the key/salt encoded as Base64, and the IV as Hex?
> - Do you think the singleton/locator pattern introduces some limitations,
such as using different encryptors in memory at the same time, either for
compatibility reasons, or legacy translating, etc?
> 
> Anyway, thanks for taking the time to respond.  I hope to learn more and
eventually contribute some back to the project, I love the activity level
and the progress being made in such an important open source project.
> 
> Thanks,
> ~Josh
> 
> 
> 
> 
> 
> 
> ________________________________
> From: Kevin W. Wall <kevin.w.wall at gmail.com>
> To: Josh Drummond <joshdrummond at yahoo.com>
> Sent: Wed, November 11, 2009 5:21:20 PM
> Subject: Re: [OWASP-ESAPI] Encryptor.encrypt/decrypt(String) deprecated?
> 
> Kevin W. Wall wrote:
>> Josh Drummond wrote:
>>> Ah yes, replacing it with something stronger yet still easy to
>>> use would be much better than removing it with no alternative.
>>> That would also align it more with the design of Encryptor.hash(String,
String)
>>> from a user point of view.
>> FWIW, you may want to take a look at the some of the doc already written
>> about the new crypto stuff.
>>
>> Go to
>>
<http://code.google.com/p/owasp-esapi-java/source/browse/#svn/trunk/document
ation>
> <...snip...>
> 
> Sent you the wrong link. This is for *browsing* the source from Subversion
> so obviously you want it to show *source* code; in this case HTML.
> 
> Instead try:
> 
>
<http://owasp-esapi-java.googlecode.com/svn/trunk/documentation/ESAPI_2.0_Re
leaseNotes_CryptoChanges.html>
> 
> and see if this makes sense. Would also appreciate any feedback, both
negative
> and positive.
> 
> Thanks,
> -kevin


-- 
Kevin W. Wall
"The most likely way for the world to be destroyed, most experts agree,
is by accident. That's where we come in; we're computer professionals.
We cause accidents."        -- Nathaniel Borenstein, co-creator of MIME