GoogleCodeExporter
commented
9 years ago My response, inline, below. -kevin
On 10/10/2010 08:15 PM, augustd wrote:
> > HI Kevin,
> >
> > Thanks for this great feedback. I still have a few thoughts on how we can
> > make something work and I am looking forward to your critique. See inline.
> >
> > -August
> >
> >
> > On Fri, Oct 8, 2010 at 8:44 PM, Kevin W. Wall <kevin.w.wall@gmail.com>wrote:
> >
>>> >>> On Fri, Oct 8, 2010 at 1:28 PM, Jeff Williams <
>>> >>> jeff.williams@aspectsecurity.com> wrote:
>>> >>>
>>>> >>>> I'm not crazy about using the word lenient. What if we just make the
>>>> >>>> method not throw an exception. It'll still get logged.
>>>> >>>>
>> >>
>> >> I don't think that logging is sufficient. See below.
>> >>
>> >> augustd wrote:
>>> >>> I agree, 'lenient' doesn't sound very 'secure'. I am open to
>> >> suggestions...
>>> >>>
>>> >>> The reason I went for a new class instead of simply changing
>>> >>> DefaultEncryptedProperties was because a new version that doesn't throw
>>> >>> those exceptions might break someone's existing code that utilizes the
>>> >>> original class. For example if they are catching those exceptions and
>> >> acting
>>> >>> upon them.
>>> >>>
>>> >>> If you don't think that is too big of an issue, I can certainly make the
>>> >>> change directly within DefaultEncryptedProperties.
>>> >>>
>> >>
>> >> No matter what you call it, I think what is being proposed is a bad
>> >> idea if the getProperty / setProperty methods do not throw anything when
>> >> the
>> >> decryption / encryption fails. I think you would at least have to throw
>> >> an unchecked exception. Here's why...
>> >>
>> >> For getProperty, if the decryption fails and you don't throw anything,
>> >> the only sensible thing to return is a null. So at some point, possibly
>> >> much later on and far from where the getProperty was originally called,
>> >> someone will try to use that (now null) value and be greeted with a
totally
>> >> unexpected NPE. That's just bad, regardless of what logging you may have,
>> >> as the affects are totally unpredictable at that point. So one can only
>> >> /hope/ people are checking for a null being returned, and I don't think
>> >> you can necessarily count on that as a defensive programming technique.
>> >>
> >
> > You could return the empty String "" and no NPE will be thrown, though you
> > still wouldn't have a useable property value.
However, there may be legitimate cases where the value of a property
may be an empty string, even for "encrypted" properties. For instance,
someone may implement something where they have something like this:
# Base64-encode secret key to use for encryption. If empty,
# generate a temporary session key and encrypt it using RSA
# and the recipient's public key and prepend the encrypted
# session key to the front of the ciphertext.
secretKey=
I'm sure that this would be rare, but why make it impossible?
If we would return an empty string from getProperty when decryption
fails, then we can no longer distinguish the two cases.
For that reason, I'd prefer to throw some sort of unchecked exception
such as EncryptionRuntimeException.
>> >> Similarly, if one calls the setProperty and the encryption fails, what do
>> >> you do? Since setProperty returns either the previously set property value
>> >> (if there was one) or null if there wasn't, using a null to indicate a
>> >> failed encryption is not going to be very helpful. Any developer who
>> >> is actually checking for null is likely to just interpret it as meaning
>> >> that there was no previous value for this property. Also, semantically,
>> >> I think you need to treat the setProperty() call as it if failed
>> >> and NOT add it to the list of properties at all rather than simply keeping
>> >> it as plaintext.
> >
> >
> > I absolutely agree.
> >
> >
> >
>> >> Otherwise, there is potential serious information
>> >> disclosure should the setProperty() call be followed by a call to
>> >> something like store() or storeToXML(). You don't want properties that
>> >> should be encrypted ending up as plaintext somewhere. True, you could
>> >> keep a list of properties that failed to encrypt properly and treat
>> >> them special [as in only log a problem] when someone tries to store
>> >> properties. But what if one were trying to store an encrypted session key?
>> >>
> >
> > Would you really be storing that in a properties file? That doesn't sound
> > very secure, encrypted or not.
Well, consider how we presently store the ESAPI default encryption key,
Encryptor.MasterKey. It is not even encrypted and it's presently
stored in ESAPI.properties. So that's even worse. (And something that
I'm going to try to address in future versions of ESAPI.)
However, I do admit that the example is a bit contrived. If it truly were
a *session* key, it should be treated as a temporary key and should not be used
to encrypt data at rest. Most likely, you are not going to persist *session*
keys at all. I was just trying to explore the consequences of the edge cases.
> >
>> >> Well, if you decided on that approach, since session keys are randomly
>> >> generated, that session key would be lost forever once the JVM exited. And
>> >> if someone had decided to encrypt something with it first, before it
>> >> was saved? Well, hope you didn't want whatever you encrypted with the
>> >> new session encryption key back.
>> >>
>> >> Now that alone is not insurmountable assuming that semantically we
>> >> just do not save the property at all...except it's quite likely that
>> >> a setProperty() call is at some point going to be followed by a
>> >> subsequent call to getProperty with the same property name, which
>> >> would in turn return null, possibly very unexpectedly. So the
>> >> NPE issue also pops up here as well.
>> >>
> >
> > Again, return "" instead.
> >
Returning an empty string for setProperty is just wrong IMHO. The Javadoc
for Properties.setProperty() clearly states that the /previous/ value is
to be returned, or null, if there was no previous value.
It's important to adhere to those semantics in case someone has code where
they have situations where they try to restore the previous value. (In fact,
I did this with ESAPI crypto JUnit tests as it was the only simple kludge I
could think of that would allow me to temporarily change the cipher
transformation and then restore it.)
But again, throwing something like a EncryptorRuntimeException when
setProperty fails to properly encrypt a value (along with obviously not
changing the value for said property) is probably acceptable.
>> >> The only way that I can see getting around this is if encryption /
>> >> decryption fails respectively in setPropery() / getProperty(), a
>> >> RuntimeException or perhaps a new EncryptionRuntimeException
>> >> would *have* to be thrown. I don't like that, but if you really
>> >> insist on having DefaultEncryptedProperties extend Properties
>> >> in a safe and secure way, I think that's what needs to be done.
>> >>
> >
> > You could extend those methods to throw RuntimeExceptions. You can still
> > check for a RuntimeException, though you don't have to. This should allow
> > for both scenarios to work.
I think as long as it's an exception that extends RuntimeException and not
actually throwing a RuntimeException, that would be acceptable.
Of course, we have to change it in the EncryptedProperties interface as well.
And once we did that, developers who are presently using these setProperty
and getProperty would have to make minor code changes to catch (say)
EncryptedRuntimeException rather than EncryptedException.
However, if DefaultEncryptedProperties is simply going to extend
java.util.Properties, one could argue why have the EncryptedProperties
interface at all. While obviously DefaultEncryptedProperties can implement
the EncryptedProperties interface as well as extend Properties, it developers
were to use the ESAPI.EncryptedProperties() locator service, they then would
not be aware of the other methods unique to Properties.
If we are going to do that, why not just build out a second reference
implementation class and allow developers to interact with it directly
through it's CTOR; for example:
Properties props = new YetAnotherEncryptedProperties();
or whatever. I don't think we want to try to mess with the EncryptedProperties
interface though (in terms of making it consistent with all the methods
in java.util.Properties); we may has well remove it instead of doing that.
>> >> Personally, I don't like it though. Of course, I'm one of the few
>> >> who seem to think that the who java.util.Properties class was wrong
>> >> from day one to *extend* java.util.Hashtable in the first place.
>> >> IMNSHO, it simply is NOT true that "Properties *is a* Hashtable"
>> >> from a semantic sense. (That seems to be acknowledged in later
>> >> versions of the JDK where you seen warnings about using things
>> >> like put() and putAll(), etc.) Similarly, I don't really think
>> >> that _semantically_ "EncryptedProperties *isA* Properties" either.
>> >> I think there are basic different intents here. Properties is something
>> >> that is meant to be shared with everyone. EncryptedProperties is something
>> >> that is only meant to be shared with a select few sharing your encryption
>> >> key. Confidentiality is not an issue with Properties; it is with
>> >> EncryptedProperties. Because of that, I just don't agree that
>> >> "EncryptedProperties isA Properties". Instead, I think composition
>> >> is the much safer choice here, just as it would have been with
>> >> Properties and Hashtable.
>> >>
>> >> Now, if you are still not convinced that DefaultEncryptedProperties
>> >> should not inherit from Properties, assume for the moment that
>> >> this is the case. Then, what should be the behavior of the two list()
>> >> methods, list(PrintSteam) and list(PrintWriter)? These two properties
>> >> are clearly aimed at troubleshooting. Even the Javadoc for both of
>> >> them says "This method is useful for debugging". So, given that
>> >> the primary use of these two methods seems to be for troubleshooting,
>> >> what should there behavior be? Should they list the encrypted values
>> >> of the properties, which is not terribly useful if you are debugging?
>> >> Or should they list the plaintext value of the encrypted properties, which
>> >> is more in line with the original debugging /intent/ of these methods?
>> >>
> >
> > Override them to throw UnsupportedOperationException. Like with any time you
> > override a base class, a careful examination of that class's existing
> > methods must be made.
> >
That is acceptable as long as it is *clearly documented* in the javadoc.
We want to keep surprises to a minimum.
So, I think I'm OK with what you propose as long as we override the two
list() methods to throw UnsupportedOperationException and clearly document
this in the Javadoc. In addition, we have setProperty() and getProperty()
methods throw some new unchecked exception (EncryptionRuntimeException ???)
in the case that encryption / decryption fails and also make it part of the
methods' declaration (even though it is not required) as well as describing
it in the Javadoc.
To me, that leaves what to do about the EncryptedProperties interface and
the ESAPI.EncryptedProperties() method. Do we keep them or remove them?
And if we keep them, do we only change EncryptedProperties so that the
declaration of setProperty / getProperty throws EncryptionRuntimException
rather than EncryptionException or do we also add add all methods from
Properties? Maybe that's something that should be summarized and put to
a vote in the ESAPI-Users mailing list?
-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
_______________________________________________
Esapi-dev mailing list
Esapi-dev@lists.owasp.org
https://lists.owasp.org/mailman/listinfo/esapi-dev
Original comment by manico.james@gmail.com on 6 Nov 2010 at 7:41
Original issue reported on code.google.com by
manico.james@gmail.comon 3 Nov 2010 at 10:01