Bugs in TLSEngine, possibly elsewhere too

141 views
Skip to first unread message

Joey

unread,
Oct 1, 2010, 3:03:29 PM10/1/10
to As3Crypto Discussion List
I've identified a bug in TLSEngine. In sendApplicationData, if the
ByteArray data is larger than 16384 bytes, but not a multiple of 16384
bytes, the application data stream can become corrupted.

The problem is that position = 0 followed by writeBytes on a ByteArray
only overwrites the data in the ByteArray, but does not change the
length except if more than length bytes are written. The correct way
to clear a byte array for re-use is clear();

So if the data parameter to this function is, say, 16385 bytes, the
result is that two records are written. The first one has a payload
of 16384 bytes. The second one should have a 1 byte payload, but
instead also has a 16384 byte payload. The other 16383 bytes are the
same as the previous record's.

This particular function can be fixed by changing this pattern:

rec.position = 0;
rec.writeBytes(...);
rec.position = 0;

into:

rec.clear();
rec.writeBytes(...);
rec.position = 0;

But I think that similar bugs based on the same misunderstanding of
ByteArray may be found elsewhere in the library. I'm about to do a
full audit of position and length usage on ByteArrays, since my
company relies heavily on as3crypto and such bugs could cause us big
problems if they aren't fixed soon. We are lucky that this one was
found in our lab before it was found in a customer site.

I'd like to request svn access so that I can commit my fixes. Would
that be acceptable?

Thanks,
--Joey Parrish

Joey Parrish

unread,
Oct 1, 2010, 4:16:38 PM10/1/10
to As3Crypto Discussion List
Embarrassingly, after searching the code, that one case is the only one I've been able to identify.  It seems to me now that the assumption was not, in fact, made that position = 0 would do what clear() does.  Before the loop was added, the use of rec would have been completely fine.  And although clear() would be correct, length = 0 followed by position = 0 would be equivalent and would keep with the style in other parts of the code.

So if you would, please commit this patch to sendApplicationData.

Sorry for flipping out a bit and making this out to be bigger than it is.  I should really make myself take the time to double-check things before I hit "send."

Thanks,
--Joey Parrish
sendApplicationData.patch

Joey Parrish

unread,
Dec 26, 2010, 4:51:53 AM12/26/10
to As3Crypto Discussion List
Ping!  Anyone out there?  Anyone with SVN access or anyone who can grant me SVN access?  I hate to think that this is a dead project already.

--Joey Parrish
Reply all
Reply to author
Forward
0 new messages