Closed
Bug 1486913
Opened 6 years ago
Closed 6 years ago
Port changes of bug 1000851 to POP and IMAP protocols
Categories
(MailNews Core :: Networking: POP, enhancement)
Tracking
(thunderbird_esr6063+ fixed, thunderbird63 fixed)
RESOLVED
FIXED
Thunderbird 63.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
(Keywords: crash, csectype-bounds, sec-low)
Attachments
(2 files, 5 obsolete files)
|
(deleted),
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1000851 +++
In bug 1000851 we fixed some issues with username and password encoding. The same issues exist in POP and IMAP and should be fixed there, too.
https://searchfox.org/comm-central/search?q=PR_snprintf(%26plain&case=false®exp=false&path=
https://searchfox.org/comm-central/rev/cce392b6a57170eb2139bbf15eaa9a88340f97b2/mailnews/local/src/nsPop3Protocol.cpp#2303
https://searchfox.org/comm-central/rev/cce392b6a57170eb2139bbf15eaa9a88340f97b2/mailnews/imap/src/nsImapProtocol.cpp#5920
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 2•6 years ago
|
||
Another déjà vu. Not truncating in IMAP won't cause crashes apart from PLAIN which has the same faulty code. I now truncate everywhere for consistency. Let me know what you think.
Attachment #9004704 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 3•6 years ago
|
||
Off by one. If you want to write 255 net bytes, you need to tell it that the buffer is 256 bytes long.
Attachment #9004687 -
Attachment is obsolete: true
Attachment #9004704 -
Attachment is obsolete: true
Attachment #9004687 -
Flags: review?(mkmelin+mozilla)
Attachment #9004704 -
Flags: review?(mkmelin+mozilla)
Attachment #9004716 -
Flags: review?(mkmelin+mozilla)
|
||
Comment 5•6 years ago
|
||
Comment on attachment 9004717 [details] [diff] [review]
1486913-auth-IMAP.patch (1b)
Review of attachment 9004717 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5942,5 @@
> ParseIMAPandCheckForNewMail();
>
> if (GetServerStateParser().LastCommandSuccessful())
> {
> + char *base64Str = PL_Base64Encode(userName, std::min((int)PL_strlen(userName), 255), nullptr);
looks like this would crash if the type casting actually is needed, and the length is suitable to make the number wrap around to the negative side
Attachment #9004717 -
Flags: review?(mkmelin+mozilla) → review-
|
|
Assignee | |
Comment 6•6 years ago
|
||
Attachment #9004716 -
Attachment is obsolete: true
Attachment #9004717 -
Attachment is obsolete: true
Attachment #9004716 -
Flags: review?(mkmelin+mozilla)
Attachment #9004785 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Comment 7•6 years ago
|
||
Sadly you didn't answer the ...
> Not truncating in IMAP won't cause crashes apart from PLAIN
> which has the same faulty code. I now truncate everywhere for consistency.
> Let me know what you think.
So if you'd rather not truncate everywhere, we need another round.
Attachment #9004787 -
Flags: review?(mkmelin+mozilla)
|
|
||
Comment 8•6 years ago
|
||
Comment on attachment 9004785 [details] [diff] [review]
1486913-auth-POP.patch (v1c)
Review of attachment 9004785 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, r=mkmelin
Attachment #9004785 -
Flags: review?(mkmelin+mozilla) → review+
|
|
||
Comment 9•6 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #7)
> I now truncate everywhere for consistency.
I think that's fine.
|
|
||
Comment 10•6 years ago
|
||
Comment on attachment 9004787 [details] [diff] [review]
1486913-auth-IMAP.patch (v1c)
Review of attachment 9004787 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5951,5 @@
> {
> ParseIMAPandCheckForNewMail(currentCommand);
> if (GetServerStateParser().LastCommandSuccessful())
> {
> + base64Str = PL_Base64Encode(password.get(), std::min((int)password.Length(), 255), nullptr);
you forgot to change these casts. ^^^, and the one above
Attachment #9004787 -
Flags: review?(mkmelin+mozilla) → review-
|
|
Assignee | |
Comment 11•6 years ago
|
||
Attachment #9004787 -
Attachment is obsolete: true
Attachment #9004791 -
Flags: review?(mkmelin+mozilla)
|
|
Assignee | |
Updated•6 years ago
|
Attachment #9004787 -
Attachment description: 1486913-auth-IMAP.patch → 1486913-auth-IMAP.patch (v1c)
|
|
||
Comment 12•6 years ago
|
||
Comment on attachment 9004791 [details] [diff] [review]
1486913-auth-IMAP.patch (1d)
Review of attachment 9004791 [details] [diff] [review]:
-----------------------------------------------------------------
Thx! r=mkmelin
Attachment #9004791 -
Flags: review?(mkmelin+mozilla) → review+
|
|
Assignee | |
Comment 13•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/271825efcfad6aec1857c88b5ef60847e0ccc37a
fix username/password handling in POP. r=mkmelin
https://hg.mozilla.org/comm-central/rev/4aace069f9d1d0952765f8490306726ad5f29c2d
fix username/password handling in IMAP. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
|
||
Updated•6 years ago
|
Group: mail-core-security → core-security-release
|
|
Assignee | |
Updated•6 years ago
|
Attachment #9004785 -
Flags: approval-comm-esr60?
|
|
Assignee | |
Updated•6 years ago
|
Attachment #9004785 -
Flags: approval-comm-esr60? → approval-comm-esr60+
|
|
Assignee | |
Comment 14•6 years ago
|
||
TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/ffe25f93f1e9ab89de12cbfded77c11108f93003
https://hg.mozilla.org/releases/comm-esr60/rev/18a94aeb864e8c847d4cc096a4e2c301f021bb5e
status-thunderbird63:
--- → fixed
status-thunderbird_esr60:
--- → fixed
tracking-thunderbird_esr60:
--- → 63+
|
|
||
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•