Repository navigation
Need major changes to configuration mechanism #180
Description
Activity
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...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-devFrom 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).From manico.james@gmail.com on May 28, 2012 20:19:33
Owner: chrisisbeef
From kevin.w.wall@gmail.com on October 31, 2013 18:32:34
Labels: Component-SecurityConfiguration
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.propertiesProblem:
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();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:
- There is no simple and safe way to override the ESAPI configuration by extending what is present.
- 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.
- There is seemingly disagree over whether or not DefaultEncryptedProperties should extend java.util.Properties.
- 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
From manico.james@gmail.com on November 03, 2010 06:01:13
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.
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