harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexey Varlamov" <alexey.v.varla...@gmail.com>
Subject Re: [jira] Commented: (HARMONY-6043) [classlib] [security] UnresolvedPermission.equals(Object) doesn't works well
Date Thu, 18 Dec 2008 13:55:16 GMT
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 <zhoukevin83@gmail.com>:
> Yes, I also agree on on the defensive copy.
>
> On Mon, Dec 15, 2008 at 11:36 AM, Tony Wu <wuyuehao@gmail.com> wrote:
>
> > On Mon, Dec 15, 2008 at 2:18 AM, Alexey Varlamov
> > <alexey.v.varlamov@gmail.com> wrote:
> > > 2008/12/13 Kevin Zhou <zhoukevin83@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
> >
>

Mime
View raw message