[Bug 39264] New: wininet may try to decrypt one more ssl message than needed
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(a)winehq.org Reporter: 7element(a)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; -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian(a)fds-team.de --- Comment #1 from Sebastian Lackner <sebastian(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #2 from 7element(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #3 from 7element(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #4 from Sebastian Lackner <sebastian(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #5 from Sebastian Lackner <sebastian(a)fds-team.de> --- *** Bug 39265 has been marked as a duplicate of this bug. *** -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 Sebastian Lackner <sebastian(a)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 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 Jacek Caban <jacek(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED CC| |jacek(a)codeweavers.com Resolution|--- |INVALID --- Comment #6 from Jacek Caban <jacek(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #7 from Jacek Caban <jacek(a)codeweavers.com> --- Also note that with the proposed patch, subsequent read_ssl_chunk would try to receive 0 bytes. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 --- Comment #8 from 7element(a)mail.bg --- Thank you very much for pointing us in the right direction. It was really helpful. You can close this as rejected. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #9 from Austin English <austinenglish(a)gmail.com> --- Closing. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=39264 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |focht(a)gmx.net Version|unspecified |1.7.51 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org