https://bugs.winehq.org/show_bug.cgi?id=39264
Bug ID: 39264 Summary: wininet may try to decrypt one more ssl message than needed Product: Wine Version: unspecified Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: wininet Assignee: wine-bugs@winehq.org Reporter: 7element@mail.bg Distribution: ---
In netconnection.c:768 in read_ssl_chunk there is no break if buf_len == ssl_buf_size. If buf_len actually becomes equal to ssl_buf_size after buf_len += size, the loop should break, otherwise a new ssl message will be tried to be read. Code needs something like this on line 769: if(buf_len == ssl_buf_size) break;
https://bugs.winehq.org/show_bug.cgi?id=39264
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian@fds-team.de
--- Comment #1 from Sebastian Lackner sebastian@fds-team.de --- You probably should have mentioned that this bug is related to https://jira.reactos.org/browse/CORE-9065, and cannot be reproduced with any of Wines default SSL implementations (well, at least not that I'm aware of). Skipping the last call to DecryptMessage just because the buffer is full doesn't seem like a correct fix.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #2 from 7element@mail.bg --- Yes, I should have mentioned that. Sorry. In my opinion if we already read the data, there is no need to try decrypt another message, but you are the expert. The other way is to check on next call to read_ssl_chunk BEFORE the assert if we already have all the data. Or change the assert to be <= and after that make the check and break. I'll leave the best course of action to your expertise, but I hope you understand why this is a problem.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #3 from 7element@mail.bg --- The reason you can't reproduce it is maybe that wine relies on much better network stack than ReactOS. But I don't think this is a bad check anyway.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #4 from Sebastian Lackner sebastian@fds-team.de --- (In reply to 7element from comment #2)
Yes, I should have mentioned that. Sorry. In my opinion if we already read the data, there is no need to try decrypt another message, but you are the expert. The other way is to check on next call to read_ssl_chunk BEFORE the assert if we already have all the data. Or change the assert to be <= and after that make the check and break. I'll leave the best course of action to your expertise, but I hope you understand why this is a problem.
The code was not written by me, and in general has a very bad quality. Nevertheless my understanding is, that an implementation can rely on the fact, that the values returned by QueryContextAttributesW are correct. This means after reading a maximum of (conn->ssl_sizes.cbHeader+conn->ssl_sizes.cbMaximumMessage+conn->ssl_sizes.cbTrailer) bytes, the implementation should be able to tell if its a correct message (SEC_E_OK) or if an error occurred, but definitely shouldn't return SEC_E_INCOMPLETE_MESSAGE anymore. As far as I know ReactOS also never encountered this assertion with the old gnutls based implementation, right?
For such development related questions and discussions it might be better to ask them at wine-devel, since its very unlikely that a lot of developers will see it here on the bugtracker.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #5 from Sebastian Lackner sebastian@fds-team.de --- *** Bug 39265 has been marked as a duplicate of this bug. ***
https://bugs.winehq.org/show_bug.cgi?id=39264
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|wininet may try to decrypt |wininet/winhttp may try to |one more ssl message than |decrypt one more ssl |needed |message than needed
https://bugs.winehq.org/show_bug.cgi?id=39264
Jacek Caban jacek@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED CC| |jacek@codeweavers.com Resolution|--- |INVALID
--- Comment #6 from Jacek Caban jacek@codeweavers.com ---
This means after reading a maximum of (conn->ssl_sizes.cbHeader+conn->ssl_sizes.cbMaximumMessage+conn->ssl_sizes. cbTrailer) bytes, the implementation should be able to tell if its a correct message (SEC_E_OK) or if an error occurred, but definitely shouldn't return SEC_E_INCOMPLETE_MESSAGE anymore.
Agreed, it's even documented by the assert. DecryptMessage should definitely be able to decrypt buffered data in this case. I'd suggest to take another look and try to fix it in ReactOS schannel.dll implementation.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #7 from Jacek Caban jacek@codeweavers.com --- Also note that with the proposed patch, subsequent read_ssl_chunk would try to receive 0 bytes.
https://bugs.winehq.org/show_bug.cgi?id=39264
--- Comment #8 from 7element@mail.bg --- Thank you very much for pointing us in the right direction. It was really helpful. You can close this as rejected.
https://bugs.winehq.org/show_bug.cgi?id=39264
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #9 from Austin English austinenglish@gmail.com --- Closing.
https://bugs.winehq.org/show_bug.cgi?id=39264
Anastasius Focht focht@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |focht@gmx.net Version|unspecified |1.7.51
-
wine-bugs＠winehq.org