You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Kevin Zhou <zh...@gmail.com> on 2008/12/13 01:43:30 UTC
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Alexey wrote,
> One must make defensive copy when setting array properties - plain
assigment of array reference is unsafe.
Yes, this assigment of arry reference is unsafe. But no spec says that the
array of certificates in a UnresovledPermission need to be thread-safe.
If we add a lock to guarantee its safe, how about the other methods which
access to targetCerts array. Do we need to make them safe too?
>Looking on the test source, I'd rather consider this as non-bug difference.
I think this is different behaviors between RI and HARMONY of determining
certificate equality?
The previous implementation of HARMONY uses PolicyUtils.matchSubSet method
which behaves differently from RI.
I think we'd better follow RI's behaviors on this.
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Tony Wu <wu...@gmail.com>.
Hi, Kevin and Alexey
I've committed this patch here[1] with a work around to NullPointerException.
[1] http://svn.apache.org/viewvc?view=rev&revision=728589
On Fri, Dec 19, 2008 at 11:42 AM, Kevin Zhou <zh...@gmail.com> wrote:
> Given a test scenario:
>
> public void test_Equals_Scenario7() {
> UnresolvedPermission up1 = new UnresolvedPermission(type, name,
> action,
> new java.security.cert.Certificate[] {cert1, cert2});
> UnresolvedPermission up2 = new UnresolvedPermission(type, name,
> action,
> new java.security.cert.Certificate[] {cert1, null, null,
> null, cert2});
> assertFalse(up1.equals(up2));
> }
>
> up1 has a 2-elements Certificate array, and up2 has a 5-elements Certificate
> array.
>
> If conducting it on the previous UnresolvedPermission, it returns true.
> Because the previous implementation ignores null-certificate variable in the
> array, then decide the equality between new Certificate[] {cert1, cert2} and
> new Certificate[] {cert1, cert2}.
> RI behave differently here, it will not take the null value in
> considerations.
>
> I agree to make a defensive copy fix in the 1st patch. Will attach it on the
> issue soon.
> Tony, would you please help to review this later?
>
--
Tony Wu
China Software Development Lab, IBM
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Kevin Zhou <zh...@gmail.com>.
Given a test scenario:
public void test_Equals_Scenario7() {
UnresolvedPermission up1 = new UnresolvedPermission(type, name,
action,
new java.security.cert.Certificate[] {cert1, cert2});
UnresolvedPermission up2 = new UnresolvedPermission(type, name,
action,
new java.security.cert.Certificate[] {cert1, null, null,
null, cert2});
assertFalse(up1.equals(up2));
}
up1 has a 2-elements Certificate array, and up2 has a 5-elements Certificate
array.
If conducting it on the previous UnresolvedPermission, it returns true.
Because the previous implementation ignores null-certificate variable in the
array, then decide the equality between new Certificate[] {cert1, cert2} and
new Certificate[] {cert1, cert2}.
RI behave differently here, it will not take the null value in
considerations.
I agree to make a defensive copy fix in the 1st patch. Will attach it on the
issue soon.
Tony, would you please help to review this later?
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Alexey Varlamov <al...@gmail.com>.
Guys, please fix at least the defensive copying and NPE in equals(),
these are plain bugs.
However, if you'd try to comply with the formal specification which
explicitly mandates copying and comparing only actual signer
certificates, you'd get back to the initial version ;)
To be honest, the spec and API is somewhat vague and seems being a
part of implicit contract with (default) Policy provider. From the
common sense, null element in certificates array are just invalid
argument to UnresovledPermission constructor which should therefore
throw an exception, but it is the way it is.
Thanks,
Alexey
2008/12/15, Kevin Zhou <zh...@gmail.com>:
> Yes, I also agree on on the defensive copy.
>
> On Mon, Dec 15, 2008 at 11:36 AM, Tony Wu <wu...@gmail.com> wrote:
>
> > On Mon, Dec 15, 2008 at 2:18 AM, Alexey Varlamov
> > <al...@gmail.com> wrote:
> > > 2008/12/13 Kevin Zhou <zh...@gmail.com>:
> > >> Alexey wrote,
> > >>> One must make defensive copy when setting array properties - plain
> > >> assigment of array reference is unsafe.
> > >> Yes, this assigment of arry reference is unsafe. But no spec says that
> > the
> > >> array of certificates in a UnresovledPermission need to be thread-safe.
> > >> If we add a lock to guarantee its safe, how about the other methods
> > which
> > >> access to targetCerts array. Do we need to make them safe too?
> > >
> > > I did not mean thread safety here, rather internal object integrity.
> > > BTW, API spec directly says: "The signer certificates are copied from
> > > the array. Subsequent changes to the array will not affect this
> > > UnsolvedPermission."
> > > The same precautions are already in place where needed.
> > >
> >
> > Alexey, I agree with you on the defensive copy.
> >
> > >>>Looking on the test source, I'd rather consider this as non-bug
> > difference.
> > >>
> > >> I think this is different behaviors between RI and HARMONY of
> > determining
> > >> certificate equality?
> > >> The previous implementation of HARMONY uses PolicyUtils.matchSubSet
> > method
> > >> which behaves differently from RI.
> > >>
> > >> I think we'd better follow RI's behaviors on this.
> > > This is better only if RI's behaviour is logical and consistent or
> > > became widely adapted feature. This is not the case here. E.g.,
> > > throwing NPE on equals() invocation is always considered bad practice,
> > > etc. Moreover, I wonder if RI is compliant with spec which says: "To
> > > determine certificate equality, this method only compares actual
> > > signer certificates. "
> > > So I suggest to rollback the patch and mark the issue "non-bug
> > difference".
> > >
> >
> > I think there is a behavior difference if we leave this gap in our
> > code. The difference is how do we do when encountering a null
> > certificate. RI takes it into consideration and Harmony just ignore
> > it. this difference itself is trival but will result in the different
> > behavior in security. For example, following two instances of
> > UnresolvedPermission are considered as equal with Harmony but just not
> > with RI.
> >
> > new UnresolvedPermission(type, name, action, new
> > java.security.cert.Certificate[]{cert1, null})
> > new UnresolvedPermission(type, name, action, new
> > java.security.cert.Certificate[]{cert1})
> >
> > I can not tell what's the impact to any product exactly at this time
> > but I'd like to mitigate the risk of creating a security hole.
> >
> > > --
> > > Alexey
> > >
> >
> >
> >
> > --
> > Tony Wu
> > China Software Development Lab, IBM
> >
>
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Kevin Zhou <zh...@gmail.com>.
Yes, I also agree on on the defensive copy.
On Mon, Dec 15, 2008 at 11:36 AM, Tony Wu <wu...@gmail.com> wrote:
> On Mon, Dec 15, 2008 at 2:18 AM, Alexey Varlamov
> <al...@gmail.com> wrote:
> > 2008/12/13 Kevin Zhou <zh...@gmail.com>:
> >> Alexey wrote,
> >>> One must make defensive copy when setting array properties - plain
> >> assigment of array reference is unsafe.
> >> Yes, this assigment of arry reference is unsafe. But no spec says that
> the
> >> array of certificates in a UnresovledPermission need to be thread-safe.
> >> If we add a lock to guarantee its safe, how about the other methods
> which
> >> access to targetCerts array. Do we need to make them safe too?
> >
> > I did not mean thread safety here, rather internal object integrity.
> > BTW, API spec directly says: "The signer certificates are copied from
> > the array. Subsequent changes to the array will not affect this
> > UnsolvedPermission."
> > The same precautions are already in place where needed.
> >
>
> Alexey, I agree with you on the defensive copy.
>
> >>>Looking on the test source, I'd rather consider this as non-bug
> difference.
> >>
> >> I think this is different behaviors between RI and HARMONY of
> determining
> >> certificate equality?
> >> The previous implementation of HARMONY uses PolicyUtils.matchSubSet
> method
> >> which behaves differently from RI.
> >>
> >> I think we'd better follow RI's behaviors on this.
> > This is better only if RI's behaviour is logical and consistent or
> > became widely adapted feature. This is not the case here. E.g.,
> > throwing NPE on equals() invocation is always considered bad practice,
> > etc. Moreover, I wonder if RI is compliant with spec which says: "To
> > determine certificate equality, this method only compares actual
> > signer certificates. "
> > So I suggest to rollback the patch and mark the issue "non-bug
> difference".
> >
>
> I think there is a behavior difference if we leave this gap in our
> code. The difference is how do we do when encountering a null
> certificate. RI takes it into consideration and Harmony just ignore
> it. this difference itself is trival but will result in the different
> behavior in security. For example, following two instances of
> UnresolvedPermission are considered as equal with Harmony but just not
> with RI.
>
> new UnresolvedPermission(type, name, action, new
> java.security.cert.Certificate[]{cert1, null})
> new UnresolvedPermission(type, name, action, new
> java.security.cert.Certificate[]{cert1})
>
> I can not tell what's the impact to any product exactly at this time
> but I'd like to mitigate the risk of creating a security hole.
>
> > --
> > Alexey
> >
>
>
>
> --
> Tony Wu
> China Software Development Lab, IBM
>
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Tony Wu <wu...@gmail.com>.
On Mon, Dec 15, 2008 at 2:18 AM, Alexey Varlamov
<al...@gmail.com> wrote:
> 2008/12/13 Kevin Zhou <zh...@gmail.com>:
>> Alexey wrote,
>>> One must make defensive copy when setting array properties - plain
>> assigment of array reference is unsafe.
>> Yes, this assigment of arry reference is unsafe. But no spec says that the
>> array of certificates in a UnresovledPermission need to be thread-safe.
>> If we add a lock to guarantee its safe, how about the other methods which
>> access to targetCerts array. Do we need to make them safe too?
>
> I did not mean thread safety here, rather internal object integrity.
> BTW, API spec directly says: "The signer certificates are copied from
> the array. Subsequent changes to the array will not affect this
> UnsolvedPermission."
> The same precautions are already in place where needed.
>
Alexey, I agree with you on the defensive copy.
>>>Looking on the test source, I'd rather consider this as non-bug difference.
>>
>> I think this is different behaviors between RI and HARMONY of determining
>> certificate equality?
>> The previous implementation of HARMONY uses PolicyUtils.matchSubSet method
>> which behaves differently from RI.
>>
>> I think we'd better follow RI's behaviors on this.
> This is better only if RI's behaviour is logical and consistent or
> became widely adapted feature. This is not the case here. E.g.,
> throwing NPE on equals() invocation is always considered bad practice,
> etc. Moreover, I wonder if RI is compliant with spec which says: "To
> determine certificate equality, this method only compares actual
> signer certificates. "
> So I suggest to rollback the patch and mark the issue "non-bug difference".
>
I think there is a behavior difference if we leave this gap in our
code. The difference is how do we do when encountering a null
certificate. RI takes it into consideration and Harmony just ignore
it. this difference itself is trival but will result in the different
behavior in security. For example, following two instances of
UnresolvedPermission are considered as equal with Harmony but just not
with RI.
new UnresolvedPermission(type, name, action, new
java.security.cert.Certificate[]{cert1, null})
new UnresolvedPermission(type, name, action, new
java.security.cert.Certificate[]{cert1})
I can not tell what's the impact to any product exactly at this time
but I'd like to mitigate the risk of creating a security hole.
> --
> Alexey
>
--
Tony Wu
China Software Development Lab, IBM
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Alexey Varlamov <al...@gmail.com>.
2008/12/13 Kevin Zhou <zh...@gmail.com>:
> Alexey wrote,
>> One must make defensive copy when setting array properties - plain
> assigment of array reference is unsafe.
> Yes, this assigment of arry reference is unsafe. But no spec says that the
> array of certificates in a UnresovledPermission need to be thread-safe.
> If we add a lock to guarantee its safe, how about the other methods which
> access to targetCerts array. Do we need to make them safe too?
I did not mean thread safety here, rather internal object integrity.
BTW, API spec directly says: "The signer certificates are copied from
the array. Subsequent changes to the array will not affect this
UnsolvedPermission."
The same precautions are already in place where needed.
>>Looking on the test source, I'd rather consider this as non-bug difference.
>
> I think this is different behaviors between RI and HARMONY of determining
> certificate equality?
> The previous implementation of HARMONY uses PolicyUtils.matchSubSet method
> which behaves differently from RI.
>
> I think we'd better follow RI's behaviors on this.
This is better only if RI's behaviour is logical and consistent or
became widely adapted feature. This is not the case here. E.g.,
throwing NPE on equals() invocation is always considered bad practice,
etc. Moreover, I wonder if RI is compliant with spec which says: "To
determine certificate equality, this method only compares actual
signer certificates. "
So I suggest to rollback the patch and mark the issue "non-bug difference".
--
Alexey
Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Posted by Kevin Zhou <zh...@gmail.com>.
How about this assignment?
if(certs != null){
targetCerts = new Certificate[certs.length];
System.arraycopy(certs, 0, targetCerts, 0, certs.length);
}