Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/mail: comments in display names are incorrectly handled #65083

Closed
rolandshoemaker opened this issue Jan 12, 2024 · 7 comments
Closed

net/mail: comments in display names are incorrectly handled #65083

rolandshoemaker opened this issue Jan 12, 2024 · 7 comments
Assignees
@rolandshoemaker
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone
Go1.23

Comments

@rolandshoemaker
Copy link
Member

In #21018, it was noted that parentheses in display names were rejected, which
resulted in a confusing error.

The solution was to introduce a behavior which diverged relatively significantly
from RFC 5322 in CL50911, allowing special reserved characters to appear in the
"atom" syntax, where they are disallowed by the specification.

One of the consequences of this change was that we introduced non-compliant
handling of comments (text within parentheses) within display names. Parsing
Hello (comment) there <hello@example.com> should result in a display name of
Hello there, but because we (a) allow special characters (in this case parentheses)
in the "atom" syntax and (b) don't properly handle comments in display names, we
parse a name of Hello (comment) there which is clearly non-conformant.

I believe we should revert CL50911, rather than just removing parentheses from
the allowed set of specials, since it's unclear why this was done to begin with,
and there is no evidence this is useful or needed.

We should additionally support the obs-phrase syntax in dispaly names, which
permit comments, and properly handle them.

Becuase this introduces a parser misalignment, which could plausibly result in
different trust decisions being made by programs using different parsers, we're
considering this a PUBLIC track security issue (per the Go Security Policy).

Thanks to Juho Nurminen of Mattermost for reporting this issue, and @Slonser for
also independently reporting this issue.
@rolandshoemaker rolandshoemaker added Security NeedsFix The path to resolution is known, but the work has not been done. labels Jan 12, 2024
@rolandshoemaker rolandshoemaker self-assigned this Jan 12, 2024
@gopherbot
Copy link

Change https://go.dev/cl/555596 mentions this issue: net/mail: properly handle special characters in phrase and obs-phrase

@Slonser
Copy link

Slonser commented Jan 13, 2024

One of the attack vectors is address spoofing

package main

import (
	"fmt"
	"net/mail"
)

func main() {
	addressListString := "(<evil@gmail.com>,) <sevakokorin80@gmail.com>"

	addressList, err := mail.ParseAddressList(addressListString)
	if err != nil {
		fmt.Println("Error parsing address list:", err)
		return
	}

	for _, addr := range addressList {
		fmt.Printf("Name: %s, Address: %s\n", addr.Name, addr.Address)
	}
}

Output:

Name: (, Address: evil@gmail.com
Name: ), Address: sevakokorin80@gmail.com

@dmitshur dmitshur added this to the Go1.23 milestone Jan 22, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@rolandshoemaker @ezz-no
net/mail: properly handle special characters in phrase and obs-phrase
21afcee 
Fixes a couple of misalignments with RFC 5322 which introduce
significant diffs between (mostly) conformant parsers.

This change reverts the changes made in CL50911, which allowed certain
special RFC 5322 characters to appear unquoted in the "phrase" syntax.
It is unclear why this change was made in the first place, and created
a divergence from comformant parsers. In particular this resulted in
treating comments in display names incorrectly.

Additionally properly handle trailing malformed comments in the group
syntax.

Fixes golang#65083

Change-Id: I00dddc044c6ae3381154e43236632604c390f672
Reviewed-on: https://go-review.googlesource.com/c/go/+/555596
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@neild
Copy link
Contributor

neild commented Feb 21, 2024

@gopherbot please open backport issues, this is a PUBLIC track security fix

This was referenced Feb 21, 2024
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #65848 (for 1.21), #65849 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/566195 mentions this issue: [release-branch.go1.21] net/mail: properly handle special characters in phrase and obs-phrase

@gopherbot
Copy link

Change https://go.dev/cl/566215 mentions this issue: [release-branch.go1.22] net/mail: properly handle special characters in phrase and obs-phrase

@neild
Copy link
Contributor

neild commented Feb 22, 2024

Assigned this CVE-2024-24784.

gopherbot pushed a commit that referenced this issue Feb 28, 2024
@rolandshoemaker @cagedmantis
[release-branch.go1.22] net/mail: properly handle special characters …
5330cd2 
…in phrase and obs-phrase

Fixes a couple of misalignments with RFC 5322 which introduce
significant diffs between (mostly) conformant parsers.

This change reverts the changes made in CL50911, which allowed certain
special RFC 5322 characters to appear unquoted in the "phrase" syntax.
It is unclear why this change was made in the first place, and created
a divergence from comformant parsers. In particular this resulted in
treating comments in display names incorrectly.

Additionally properly handle trailing malformed comments in the group
syntax.

For #65083
Fixed #65849

Change-Id: I00dddc044c6ae3381154e43236632604c390f672
Reviewed-on: https://go-review.googlesource.com/c/go/+/555596
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/566215
Reviewed-by: Carlos Amedee <carlos@golang.org>
gopherbot pushed a commit that referenced this issue Feb 28, 2024
@rolandshoemaker @cagedmantis
[release-branch.go1.21] net/mail: properly handle special characters …
263c059 
…in phrase and obs-phrase

Fixes a couple of misalignments with RFC 5322 which introduce
significant diffs between (mostly) conformant parsers.

This change reverts the changes made in CL50911, which allowed certain
special RFC 5322 characters to appear unquoted in the "phrase" syntax.
It is unclear why this change was made in the first place, and created
a divergence from comformant parsers. In particular this resulted in
treating comments in display names incorrectly.

Additionally properly handle trailing malformed comments in the group
syntax.

For #65083
Fixes #65848

Change-Id: I00dddc044c6ae3381154e43236632604c390f672
Reviewed-on: https://go-review.googlesource.com/c/go/+/555596
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/566195
Reviewed-by: Carlos Amedee <carlos@golang.org>
bradfitz pushed a commit to tailscale/go that referenced this issue Mar 5, 2024
@rolandshoemaker @bradfitz
[release-branch.go1.22] net/mail: properly handle special characters …
5c01bab 
…in phrase and obs-phrase

Fixes a couple of misalignments with RFC 5322 which introduce
significant diffs between (mostly) conformant parsers.

This change reverts the changes made in CL50911, which allowed certain
special RFC 5322 characters to appear unquoted in the "phrase" syntax.
It is unclear why this change was made in the first place, and created
a divergence from comformant parsers. In particular this resulted in
treating comments in display names incorrectly.

Additionally properly handle trailing malformed comments in the group
syntax.

For golang#65083
Fixed golang#65849

Change-Id: I00dddc044c6ae3381154e43236632604c390f672
Reviewed-on: https://go-review.googlesource.com/c/go/+/555596
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/566215
Reviewed-by: Carlos Amedee <carlos@golang.org>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Mar 6, 2024
@rolandshoemaker @romaindoumenc
[release-branch.go1.22] net/mail: properly handle special characters …
ee82336 
…in phrase and obs-phrase

Fixes a couple of misalignments with RFC 5322 which introduce
significant diffs between (mostly) conformant parsers.

This change reverts the changes made in CL50911, which allowed certain
special RFC 5322 characters to appear unquoted in the "phrase" syntax.
It is unclear why this change was made in the first place, and created
a divergence from comformant parsers. In particular this resulted in
treating comments in display names incorrectly.

Additionally properly handle trailing malformed comments in the group
syntax.

For golang#65083
Fixed golang#65849

Change-Id: I00dddc044c6ae3381154e43236632604c390f672
Reviewed-on: https://go-review.googlesource.com/c/go/+/555596
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/566215
Reviewed-by: Carlos Amedee <carlos@golang.org>
@blackgnezdo blackgnezdo mentioned this issue Mar 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolandshoemaker rolandshoemaker

Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
5 participants
@neild @dmitshur @rolandshoemaker @gopherbot @Slonser