[Android] Prevent potential out of bounds write on Android #62275
Conversation
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue Detailsnull
|
| { | ||
| data = make_java_byte_array(env, rem); | ||
| int32_t bytes_to_read = rem < length ? rem : length; | ||
| assert(bytes_to_read == rem && "destination buffer is not large enough"); |
There was a problem hiding this comment.
Is this assert a debug-only assert, or a release-and-debug assert?
If it's debug only, what state would we be in if it hit in the real world? I think it'd be data loss, since I think ByteBuffer.compact will throw away the excess data.
There was a problem hiding this comment.
This assert is debug-only. I don't believe the code would lose data even if it ran into this edge case because compact removes only the data that was already transferred into data and it would move the remaining data to the beginning of the appInBuffer.
From what I can tell, the function is currently used to read all of the data at once (the sizes of the input and output buffer are the same and it is assumed that the size of the output is never larger than the input) and it never checks if there are any bytes left in the input buffer after this function is used. These assumptions might not hold if somebody decides to reuse this function in the future though.
There was a problem hiding this comment.
The second option would be to avoid reading any data from the buffer and report SSLStreamStatus_Error. Maybe that is actually a better option and it also won't affect the current use on this function.
There was a problem hiding this comment.
removes only the data that was already transferred into data
Ah, right, since the destination array is smaller it'll bound the read. That's probably reasonable, then.
If we're handling the case, we can probably drop the assert and let it just run.
There was a problem hiding this comment.
I doublechecked the series of ByteBuffer operations and you can obtain all of the data by repeatedly calling AndroidCryptoNative_SSLStreamRead. The behavior is now the same in the other two PAL implementations (AppleCrypto, CryptoNative) and it matches the description in pal_sslstream.h ("Unless data from a previous incomplete read is present, this will invoke the STREAM_READER callback."). The "second option" would not be a good idea after all.
I think this PR is ready to be merged. We unfortunately don't have any unit tests for the PAL layer AFAIK.
|
Tagging subscribers to 'arch-android': @steveisok, @akoeplinger Issue DetailsThis PR makes sure we never write more bytes into the destination buffer than we allocated. The way we currently use this function always is safe because we ensure that the destination buffer is large enough beforehand but it could be a problem if we reused this function somewhere else in the future. If there's too much data in the input buffer to fit into the destination buffer we read only as much as we can and we leave the rest of the data intact for future reads.
|
|
@bartonjs Can you please review when you have a moment? |
|
/backport to release/6.0-maui |
|
Started backporting to release/6.0-maui: https://github.com/dotnet/runtime/actions/runs/1707077297 |
ghost
locked as resolved and limited conversation to collaborators
5 participants
This PR makes sure we never write more bytes into the destination buffer than we allocated. The way we currently use this function always is safe because we ensure that the destination buffer is large enough beforehand but it could be a problem if we reused this function somewhere else in the future.
If there's too much data in the input buffer to fit into the destination buffer we read only as much as we can and we leave the rest of the data intact for future reads.