Need major changes to configuration mechanism

[meg23]

From [email protected] 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 [email protected] 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 [email protected]:

On Fri, Oct 8, 2010 at 1:28 PM, Jeff Williams < [email protected]> 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 [email protected] 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 [email protected] https://lists.owasp.org/mailman/listinfo/esapi-dev

[meg23]

From [email protected] 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 [email protected] on May 28, 2012 20:19:33

Owner: chrisisbeef

[meg23]

From [email protected] on October 31, 2013 18:32:34

Labels: Component-SecurityConfiguration

[meg23]

From [email protected] 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();

[kwwall]

Wow, reading through this, I'm not really sure what do do with this (other than to remove the 'Milestone-Release2.1' label and replace this this with 'milestone 3.0', which I've done). I have also changed it from 'bug' to 'enhancement', although 'design flaw' (if we had such a label) would probably be more accurate.

That said, this GitHub issue seems to be about a multitude of things, most of which are IMO, caused by the overzealous use of the singleton design pattern:

1. There is no simple and safe way to override the ESAPI configuration by extending what is present.
2. There is no way to support a different set ESAPI properties for one usage and a different one later in some different place. For example, if an application were using the ESAPI Encoder reference implementation, but in one specific place the application needed to allow multiple encodings but wanted to disallow them everywhere else, the application would be hard pressed to do so because there is a single ESAPI property (Encoder.AllowMultipleEncoding) that is looked at only once and never afterwards. Since the current reference implementation of DefaultEncoder is designed and implemented as a singleton, this behavior makes sense, but it also restricts the use of ESAPI when there comes a need to support multiple values for the same ESAPI property.
3. There is seemingly disagree over whether or not DefaultEncryptedProperties should extend java.util.Properties.
4. Probably other important things that I haven't listed.

Before someone runs off to implement something, I propose that a bit of good, old fashioned analysis be done. Minimally, some use cases that must be supported (as well as those that should be supported) should be written up and discussed. Then some APIs with javadoc and some example concept code how it would be used should be provided for discussion. It is only after both of these things happen should we dive into to implementation.

Because all of this would break interfaces for 2.x backward compatibility, I am marking this as a 3.0 milestone where such backward compatibility is not being promised. and wished to