Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
> I still need an official reviewer. Thanks for looking into this, I was going check into it today if you didn't. I figured it must be something in byte comparison. Sure enough. Good catches! :) That code's been in there a long time! Only nit is Copyright Dates if you choose to update. Rest looks good. Brad On 5/6/2014 6:43 AM, Xuelei Fan wrote: On 5/6/2014 9:39 PM, Florian Weimer wrote: On 05/06/2014 03:37 PM, Xuelei Fan wrote: On 5/6/2014 9:31 PM, Florian Weimer wrote: On 05/06/2014 02:00 PM, Xuelei Fan wrote: Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. Yes, I get that. I've verified that you've covered all the version comparisons. Thanks for the code review. Do you have a OpenJDK author account? I'm not an official reviewer, I'm afraid. I will your name in a "also reviewed by" section. I still need an official reviewer. Thanks, Xuelei
Re: Review Request of JDK Enhancement Proposal: OCSP stapling
On 5/6/2014 9:36 PM, Florian Weimer wrote: > On 04/02/2014 01:19 AM, Xuelei Fan wrote: >> Here is the updated version: >>http://cr.openjdk.java.net/~xuelei/8034248/jep-csre-v01.txt >> >> Updated the description section and a few words so that it is easier to >> understand. > > I think the server side would benefit from an API which allows code to > directly supply the OCSP response to be stapled, perhaps as part of the > extended trust manager. > Typically, OCSP response is time-variant. Ideally, the response should be retrieved and updated internally, in time and automatically. For the first stage, I only want to implement the essential feature, and keep the footprint as small as possible. Thanks, Xuelei
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 5/6/2014 9:31 PM, Florian Weimer wrote: > On 05/06/2014 02:00 PM, Xuelei Fan wrote: > >> Storing both int version and major/minor byte versions is a little bit >> redundancy. The update is significant. I will focus on the signed byte >> issue in this fix. > > Yes, I get that. I've verified that you've covered all the version > comparisons. > Thanks for the code review. Do you have a OpenJDK author account? > I was just wondering if accessor methods (or storing the values as ints) > would make it less likely that the issue reoccurs in a different > variant. I think, it should be better. Thanks, Xuelei > But the new checkRecordVersion() probably covers that. >
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 05/06/2014 03:37 PM, Xuelei Fan wrote: On 5/6/2014 9:31 PM, Florian Weimer wrote: On 05/06/2014 02:00 PM, Xuelei Fan wrote: Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. Yes, I get that. I've verified that you've covered all the version comparisons. Thanks for the code review. Do you have a OpenJDK author account? I'm not an official reviewer, I'm afraid. I used to have an author account (fweimer), but can no longer access it. -- Florian Weimer / Red Hat Product Security Team
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 5/6/2014 9:39 PM, Florian Weimer wrote: > On 05/06/2014 03:37 PM, Xuelei Fan wrote: >> On 5/6/2014 9:31 PM, Florian Weimer wrote: >>> On 05/06/2014 02:00 PM, Xuelei Fan wrote: >>> Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. >>> >>> Yes, I get that. I've verified that you've covered all the version >>> comparisons. > >> Thanks for the code review. Do you have a OpenJDK author account? > > I'm not an official reviewer, I'm afraid. > I will your name in a "also reviewed by" section. I still need an official reviewer. Thanks, Xuelei
Re: Review Request of JDK Enhancement Proposal: OCSP stapling
On 04/02/2014 01:19 AM, Xuelei Fan wrote: Here is the updated version: http://cr.openjdk.java.net/~xuelei/8034248/jep-csre-v01.txt Updated the description section and a few words so that it is easier to understand. I think the server side would benefit from an API which allows code to directly supply the OCSP response to be stapled, perhaps as part of the extended trust manager. -- Florian Weimer / Red Hat Product Security Team
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 05/06/2014 02:00 PM, Xuelei Fan wrote: Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. Yes, I get that. I've verified that you've covered all the version comparisons. I was just wondering if accessor methods (or storing the values as ints) would make it less likely that the issue reoccurs in a different variant. But the new checkRecordVersion() probably covers that. -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH] Add class java.security.StandardMessageDigests
On 5/6/2014 9:01 PM, Florian Weimer wrote: >> When the implementation of the underlying is unknown, it is hard to >> estimate the detailed behavior in the unknown black box. > > True. So you think providing more efficient means for hashing > relatively short byte sequences isn't worth the effort? The benefits are small, I think. It is easy to get similar improvement in application layer, but if we want add this to JRE for general use, we may need to evaluation too much uncertainties or limitations. Xuelei
Re: [PATCH] Add class java.security.StandardMessageDigests
On 05/06/2014 02:35 PM, Xuelei Fan wrote: When I though about the case, the idea come to my mind was that the clone() may need to use the current states of MD. It is great if all of the current states can also be cloned to another session. But ... The PKCS#11 provider can do this. The session is initialized lazily, so that it doesn't have to be cloned at all if no data has been hashed yet. As far as I can tell, you end up with the same token no matter how the digest object is created (after all, there might be only one in the system), which probably incurs some synchronization overhead. But it doesn't matter if you use cloning or not. Allocation is always kind of expensive because of the finalization requirement. When the implementation of the underlying is unknown, it is hard to estimate the detailed behavior in the unknown black box. True. So you think providing more efficient means for hashing relatively short byte sequences isn't worth the effort? -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH] Add class java.security.StandardMessageDigests
>> Per the spec, clone() may throw CloneNotSupportedException. It is OK a >> certain provider does not support Cloneable. > > The key part is that the behavior has to be consistent across all > objects. It's not required that clone() works, but if it works for one > instance, it works for all of them. > True. >>> The >>> TLS implementation also relies on this behavior (i.e. clone() either >>> consistently fails or consistently succeeds). >>> >> That's true. SunJSSE requires cloneable MD implementation. > > Are you sure? It contains a fairly elaborate workaround for the > non-cloneable case (construct as many digest objects as needed, and then > feed them data in parallel so that you can finalize one pseudo-clone, > but continue hashing using the other one). > Right. You got it! That's also what I mean with "cloneable". BTW! I guess in some situation or some providers, clone() might not be a lightweight operation. >>> >>> Hmm. I can see that the state cannot be cloned at all (that's why >>> cloning is optional), but allocating a new state has to happen anyway, >>> no matter how the object is constructed. >>> >> Let's consider a case, every MD object should be bound to a session, and >> the operations should be synchronized in the session. clone() will share >> the session, the operations among different MD objects that share the >> session need to be synchronized. I think, there is a significant >> performance and scalablity impact if StandardMessageDigests is used for >> such cases in concurrency context. > > Why would clone() share the session, but constructing a new digest would > not? Because sharing the session would be the only way to share the > state? Ugh, yes, you might be right. > When I though about the case, the idea come to my mind was that the clone() may need to use the current states of MD. It is great if all of the current states can also be cloned to another session. But ... When the implementation of the underlying is unknown, it is hard to estimate the detailed behavior in the unknown black box. Xuelei
Re: [PATCH] Add class java.security.StandardMessageDigests
On 05/06/2014 01:39 PM, Xuelei Fan wrote:
Another concern of mine is about that the returned MessageDigest object
heavily depends on the providers configuration when the object get
instantiated for the 1st time. If the providers configuration get
updated during run time, the provider of the returned MessageDigest
object does not get updated accordingly. This proposal may be a pretty
good practice in application layer in some circumstance, but may result
in unnecessary complexity in JRE layer because we don't know the exact
context that the application want to use this class, and the exact
implementation details of the underlying MD service provider.
Hmm. It would certainly be possible to invalidate the cache if the
provider configuration is changed.
It's unlikely that a library-based reimplementation could get this right.
Per the spec, clone() may throw CloneNotSupportedException. It is OK a
certain provider does not support Cloneable.
The key part is that the behavior has to be consistent across all
objects. It's not required that clone() works, but if it works for one
instance, it works for all of them.
The
TLS implementation also relies on this behavior (i.e. clone() either
consistently fails or consistently succeeds).
That's true. SunJSSE requires cloneable MD implementation.
Are you sure? It contains a fairly elaborate workaround for the
non-cloneable case (construct as many digest objects as needed, and then
feed them data in parallel so that you can finalize one pseudo-clone,
but continue hashing using the other one).
If the performance can get improvement here by define a static cloneable
object, it might be also can get improvement in the same way in the
provider implementation, unless the improvement is all about
implementation service look up among the supported providers.
It's the service lookup. Well, it was the service lookup in 2010.
BTW! I guess in some situation or some providers, clone() might not be
a lightweight operation.
Hmm. I can see that the state cannot be cloned at all (that's why
cloning is optional), but allocating a new state has to happen anyway,
no matter how the object is constructed.
Let's consider a case, every MD object should be bound to a session, and
the operations should be synchronized in the session. clone() will share
the session, the operations among different MD objects that share the
session need to be synchronized. I think, there is a significant
performance and scalablity impact if StandardMessageDigests is used for
such cases in concurrency context.
Why would clone() share the session, but constructing a new digest would
not? Because sharing the session would be the only way to share the
state? Ugh, yes, you might be right.
What I meant is that there might be no place to store the reference to a
long-term MessageDigest object which is reused again and again, so the
best thing the code can do is to allocate a fresh object when it is needed?
The object of MessageDigest is not immutable. IMHO, as we discussed
above, there are a lot of issues need to be considered carefully to
support such a long-term object in JRE.
I meant a method like this:
public static byte[] sha1(byte[] buffer, int offset, int length) {
MessageDigest md = threadLocalMDs.sha1();
md.update(buffer, offset, length);
return md.digest();
}
The actual digest object would not be exposed through this interface.
This would cover the use case of hashing short buffers, where the
instantiation overhead is most visible.
--
Florian Weimer / Red Hat Product Security Team
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 5/6/2014 7:41 PM, Florian Weimer wrote: > On 05/06/2014 12:10 PM, Xuelei Fan wrote: > >> Please review this simple but interesting fix: >>http://cr.openjdk.java.net/~xuelei/8042449/webrev.00/ > > It's strange that the code caches the major/minor version components as > fields of ProtocolVersion, but these fields cannot be used directly and > still need shifting and masking. > > Maybe it's better to store the components as ints, or remove the fields > completely and use accessors that extract the components from the > version field as needed? > We need to convert the byte and int type back-and-forth. Store as ints is convenient for comparing. Store as bytes is convenient for assigning to a new generated record. Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. Thanks for the comment! Xuelei
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
On 05/06/2014 12:10 PM, Xuelei Fan wrote: Please review this simple but interesting fix: http://cr.openjdk.java.net/~xuelei/8042449/webrev.00/ It's strange that the code caches the major/minor version components as fields of ProtocolVersion, but these fields cannot be used directly and still need shifting and masking. Maybe it's better to store the components as ints, or remove the fields completely and use accessors that extract the components from the version field as needed? -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH] Add class java.security.StandardMessageDigests
Another concern of mine is about that the returned MessageDigest object heavily depends on the providers configuration when the object get instantiated for the 1st time. If the providers configuration get updated during run time, the provider of the returned MessageDigest object does not get updated accordingly. This proposal may be a pretty good practice in application layer in some circumstance, but may result in unnecessary complexity in JRE layer because we don't know the exact context that the application want to use this class, and the exact implementation details of the underlying MD service provider. On 5/6/2014 5:45 PM, Florian Weimer wrote: > On 05/05/2014 04:43 PM, Xuelei Fan wrote: > >>> It's 10% faster, even including the digest overhead for a >>> single-block message. > >> clone() is an optional operation for MD. This point may make this >> class unreliable. > > I think the MessageDigest specification requires that this works. Per the spec, clone() may throw CloneNotSupportedException. It is OK a certain provider does not support Cloneable. > The > TLS implementation also relies on this behavior (i.e. clone() either > consistently fails or consistently succeeds). > That's true. SunJSSE requires cloneable MD implementation. > Note that I'm not blindly calling clone(), I'm providing a fallback in > case clone() does not work. > It's a good workaround. >> I think it might be a better place to make the improvement in crypto >> providers. > > What kind of improvement are you thinking of? > If the performance can get improvement here by define a static cloneable object, it might be also can get improvement in the same way in the provider implementation, unless the improvement is all about implementation service look up among the supported providers. >> BTW! I guess in some situation or some providers, clone() might not be >> a lightweight operation. > > Hmm. I can see that the state cannot be cloned at all (that's why > cloning is optional), but allocating a new state has to happen anyway, > no matter how the object is constructed. > Let's consider a case, every MD object should be bound to a session, and the operations should be synchronized in the session. clone() will share the session, the operations among different MD objects that share the session need to be synchronized. I think, there is a significant performance and scalablity impact if StandardMessageDigests is used for such cases in concurrency context. >>> I think the single-block hashing case is important because it is not >>> always possible to reuse an existing digest object after calling >>> reset(). > >> It means the reset() does not work. Looks like a bug to me. > > What I meant is that there might be no place to store the reference to a > long-term MessageDigest object which is reused again and again, so the > best thing the code can do is to allocate a fresh object when it is needed? > The object of MessageDigest is not immutable. IMHO, as we discussed above, there are a lot of issues need to be considered carefully to support such a long-term object in JRE. Thanks, Xuelei > Hmm, maybe we could provide a tread-local message digest that can be > used to hash byte arrays? It would address the simplest possible case > where the instantiation overhead is most visible. >
Code review request [JDK 9] 8042449 Issue for negative byte major record version
Hi,
Please review this simple but interesting fix:
http://cr.openjdk.java.net/~xuelei/8042449/webrev.00/
During the checking of invalid record version, a byte to byte comparing
is coded as:
if (...
recordVersion.major > ProtocolVersion.MAX.major) {
throw new SSLException
}
"recordVersion.major" and "ProtocolVersion.MAX.major" is byte type,
which is signed. If the major version is "0xa9", recordVersion.major is
a negative value (-87). If ProtocolVersion.MAX.major is positive, the
checking above does not work any more.
This fix converts the version number to positive value before make the
comparing.
Thanks,
Xuelei
Re: [PATCH] Add class java.security.StandardMessageDigests
On 05/05/2014 04:43 PM, Xuelei Fan wrote: It's 10% faster, even including the digest overhead for a single-block message. clone() is an optional operation for MD. This point may make this class unreliable. I think the MessageDigest specification requires that this works. The TLS implementation also relies on this behavior (i.e. clone() either consistently fails or consistently succeeds). Note that I'm not blindly calling clone(), I'm providing a fallback in case clone() does not work. I think it might be a better place to make the improvement in crypto providers. What kind of improvement are you thinking of? BTW! I guess in some situation or some providers, clone() might not be a lightweight operation. Hmm. I can see that the state cannot be cloned at all (that's why cloning is optional), but allocating a new state has to happen anyway, no matter how the object is constructed. I think the single-block hashing case is important because it is not always possible to reuse an existing digest object after calling reset(). It means the reset() does not work. Looks like a bug to me. What I meant is that there might be no place to store the reference to a long-term MessageDigest object which is reused again and again, so the best thing the code can do is to allocate a fresh object when it is needed? Hmm, maybe we could provide a tread-local message digest that can be used to hash byte arrays? It would address the simplest possible case where the instantiation overhead is most visible. -- Florian Weimer / Red Hat Product Security Team
