Need major changes to configuration mechanism

[meg23]

From manico.james@gmail.com on November 03, 2010 06:01:13

1. the requirement to call
   ESAPI.override( new DefaultSecurityConfiguration() );
   which according to the Javadoc says:
   Overrides the current security configuration with a new
   implementation. This is meant to be used as a temporary means to alter
   the behavior of the ESAPI and should NEVER be used in a production
   environment as it will affect the behavior and configuration of the
   ESAPI GLOBALLY.

so right off the bat we are telling folks that if they need to use this,
that you should just ignore the warnings in the Javadoc. If the Javadoc
*is* accurate, then we are setting a bad precedent, telling developers
to "pay no attention to the warning labels". If the Javadoc is not accurate,
then at a minimum, let's fix the Javadoc as playing Chicken Little is almost
equally bad.

2. DefaultSecurityConfiguration is designed as a singleton. Unfortunately, it
   is a poorly designed / implemented singleton as it has TWO public CTORs!
   I think this is just asking for problems. Is it supposed to be a singleton or
   not. If it is, the CTORs should be private (and there is little reason for
   two of them). If it doesn't need to be a singleton, then I could stand behind
   these changes if DefaultSecurityConfiguration was rewritten so that it did
   not have to be a singleton. (I did not reinspect its source code that
   carefully, so for the moment at least, I am not making any judgment how
   difficult this is.) But I don't think we can have it both ways. If it is
   supposed to be a singleton, then calling a public CTOR could have undesired
   side-effects. Has anyone checked to see if this is OK? \

Original issue: http://code.google.com/p/owasp-esapi-java/issues/detail?id=170

[meg23]

From manico.james@gmail.com on November 06, 2010 00:41:53

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

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 re...

[meg23]

From manico.james@gmail.com on November 06, 2010 00:41:53

...ally

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

[meg23]

From manico.james@gmail.com on November 06, 2010 01:28:05

A few observations on this...
1) I have been working on ESAPI 2.0 for more than a year now and
I do not recall ever seeing in ESAPI 2.0 (I started with rc2, I
think) a mechanism to automatically reload ESAPI.properties or
validation.properties if one of those had been changed.
2) To make re-loading of ESAPI.properties useful, we need to ensure
certain semantics are consistency enforced across all of ESAPI.
Without this, we will get inconsistent results that can result in
security vulnerabilities. And the first step is to clearly state
what these semantics ought to be from a requirements point of view.
Secondly, one would need to write the reference implementation in a
certain manner to enforce these semantics. Currently, I am not convinced
that this is done across ESAPI with any sort of consistency. So right
now most of you are probably scratching your head and asking what on
earth is he talking about. Let me illustrate.

For instance, suppose there were some ESAPI reference class
org.owasp.esapi.reference.DefaultXyz that initializes an instance
variable that it uses something like this:

    private final String someProperty =
        ESAPI.securityConfiguration().getSomeProperty();

and then this property is used throughout reference class Xyz. So
changing whatever property DefaultSecurityConfiguration is checking
is only going to affect NEW instances of class Xyz.  OTOH, if class
Xyz is implemented as a singleton, then all instances are the same,
so changing this specific property in ESAPI.properties would ultimately
have no affect even if the DefaultSecurityConfiguration re-parsed
the modified ESAPI.properties file and subsequent calls to
DefaultSecurityConfiguration.getSomeProperty() were to return the
updated value.

Now of course, there is another way to write this code and that
is not to use an instance variable to hold said property at all,
but to simply call ESAPI.securityConfiguration().getSomeProperty()
whenever it is needed.  But notice that this would give us slightly
different semantics. Now we pick up the new value from a modified
ESAPI.properties whenever that property is used. And while this has
the same semantics for new instances, it acts differently for existing
instances of class Xyz. Perhaps more importantly, it now acts
differently in the case of reference classes implemented as singletons.

Now, you might be saying, "what's the big deal?"  But if you change a
property in ESAPI.properties and ESAPI is designed to re-load / re-parse
if its modification time changes, then you would *probably* expect it to
act in some uniform manner. I am just saying that you cannot necessarily
assume that because that's not the semantics that most of ESAPI was
necessarily implemented under.

 3) Because of #2 above, rather than relying on changing ESAPI.properties to
affect the behavior of a specific class instance, I would like to see
(for non-singleton classes) the addition of new class CTOR that is
like this
    public SomeClass(Properties props)
where the properties defined in 'props' would overwrite those set in
ESAPI.properties. Of course, the rest of the class needs to be
appropriately written to take advantage of this as well. IMO, it doesn't
make sense for classes implemented as singletons though.

Anyway, bottom line, before we jump into this, we first need to define and agree
upon requirements, and secondly, make sure that properties are all used
consistent with whatever semantics we decide we want. I think that if we
decide to do this, it should be postponed to the next release (2.1 or 3.0
or whatever follows).

[meg23]

From manico.james@gmail.com on May 28, 2012 20:19:33

Owner: chrisisbeef

[meg23]

From kevin.w.wall@gmail.com on October 31, 2013 18:32:34

Labels: Component-SecurityConfiguration

[meg23]

From jmaNever...@gmail.com on December 20, 2013 11:46:41

Here's just a bit more fuel for the fire.
With WebSphere Java 2 Security turned on we got:
java.security.AccessControlException: Access denied (java.io.FilePermission C:\WINDOWS\system32\config\systemprofile.esapi\ESAPI.properties

Problem:
DefaultSecurityConfiguration
private Properties loadConfigurationFromClasspath()
ClassLoader[] loaders = new ClassLoader[] {
ClassLoader.getSystemClassLoader(),

To allow any to be null I changed it in our own delegate class to:
ClassLoader[] loaders = new ClassLoader[3];
loaders[0] = getContextLoader();
loaders[1] = getSystemLoader();
loaders[2] = getThisLoader();