* Matthew Toseland <toad at amphibian.dyndns.org> [2008-04-14 22:49:22]:
> On Monday 14 April 2008 06:28, nextgens at freenetproject.org wrote:
> > Author: nextgens
> > Date: 2008-04-14 05:28:57 +0000 (Mon, 14 Apr 2008)
> > New Revision: 19303
> >
> > Modified:
> > trunk/freenet/src/freenet/keys/ClientCHKBlock.java
> > trunk/freenet/src/freenet/node/FNPPacketMangler.java
> > trunk/freenet/src/freenet/node/LocationManager.java
> > Log:
> > fix two potential security weaknesses (Would thow ArrayOutOfBounds)
> >
> > Modified: trunk/freenet/src/freenet/keys/ClientCHKBlock.java
> > ===================================================================
> > --- trunk/freenet/src/freenet/keys/ClientCHKBlock.java 2008-04-14
> > 04:29:38
> UTC (rev 19302)
> > +++ trunk/freenet/src/freenet/keys/ClientCHKBlock.java 2008-04-14
> > 05:28:57
> UTC (rev 19303)
> > @@ -89,8 +89,8 @@
> > throw new CHKDecodeException("Crypto key too short");
> > cipher.initialize(key.cryptoKey);
> > PCFBMode pcfb = PCFBMode.create(cipher);
> > - byte[] hbuf = new byte[headers.length-2];
> > - System.arraycopy(headers, 2, hbuf, 0, headers.length-2);
> > + byte[] hbuf = new byte[headers.length > 2 ? headers.length-2 : 0];
> > + System.arraycopy(headers, 2, hbuf, 0, hbuf.length);
>
> I disagree. We should throw. And we do, in the CHKBlock constructor. Please
> revert.
Okay, for some reason I've missed the test in the constructor :$
Still, as CHKBlock isn't "final" I think that we could do an extra test here.
>
> > byte[] dbuf = new byte[data.length];
> > System.arraycopy(data, 0, dbuf, 0, data.length);
> > // Decipher header first - functions as IV
> >
> > Modified: trunk/freenet/src/freenet/node/FNPPacketMangler.java
> > ===================================================================
> > --- trunk/freenet/src/freenet/node/FNPPacketMangler.java 2008-04-14
> > 04:29:38
> UTC (rev 19302)
> > +++ trunk/freenet/src/freenet/node/FNPPacketMangler.java 2008-04-14
> > 05:28:57
> UTC (rev 19303)
> > @@ -1124,7 +1124,7 @@
> > byte[] data = new byte[decypheredPayload.length -
> decypheredPayloadOffset];
> > System.arraycopy(decypheredPayload, decypheredPayloadOffset,
> > data, 0,
> decypheredPayload.length - decypheredPayloadOffset);
> > long bootID = Fields.bytesToLong(data);
> > - byte[] hisRef = new byte[data.length -8];
> > + byte[] hisRef = new byte[data.length > 8 ? data.length -8 : 0];
>
> Here we definitely shouldn't swallow it silently, we should log it and kill
> the packet, as we do with other formatting errors. We should not allocate a
> zero length array and then have devs running in circles trying to figure out
> what's going on!
Fixed
>
> > System.arraycopy(data, 8, hisRef, 0, hisRef.length);
> >
> > // construct the peernode
> >
> > Modified: trunk/freenet/src/freenet/node/LocationManager.java
> > ===================================================================
> > --- trunk/freenet/src/freenet/node/LocationManager.java 2008-04-14
> > 04:29:38
> UTC (rev 19302)
> > +++ trunk/freenet/src/freenet/node/LocationManager.java 2008-04-14
> > 05:28:57
> UTC (rev 19303)
> > @@ -349,7 +349,7 @@
> > }
> > registerKnownLocation(hisLoc);
> >
> > - double[] hisFriendLocs = new double[hisBufLong.length-2];
> > + double[] hisFriendLocs = new double[hisBufLong.length > 2 ?
> hisBufLong.length-2 : 0];
> > for(int i=0;i<hisFriendLocs.length;i++) {
> > hisFriendLocs[i] =
> Double.longBitsToDouble(hisBufLong[i+2]);
> > if((hisFriendLocs[i] < 0.0) || (hisFriendLocs[i] > 1.0)) {
> > @@ -546,7 +546,7 @@
> > }
> > registerKnownLocation(hisLoc);
> >
> > - double[] hisFriendLocs = new double[hisBufLong.length-2];
> > + double[] hisFriendLocs = new double[hisBufLong.length > 2
> > ?
> hisBufLong.length-2 : 0];
>
> Likewise here, we should do proper error handling. It's not that hard.
Is it acceptable for a swaprequest not to have any 'friend location' ?
See what I've commited in r19342
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<https://emu.freenetproject.org/pipermail/devl/attachments/20080415/081be6a3/attachment.pgp>
