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.

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

Auth is ignored in the master branch #171

Closed
kayrus opened this issue Nov 29, 2021 · 3 comments
Closed

Auth is ignored in the master branch #171

kayrus opened this issue Nov 29, 2021 · 3 comments

Comments

@kayrus
Copy link
Contributor

kayrus commented Nov 29, 2021

@foxcpp the #146 PR introduced a breaking change. While adapting my code to meet the recent package requirements I noticed that AUTH can be ignored.

A simple server code example avoids auth:

server side output:

$ go run main.go
2021/11/26 19:45:31 Starting server at :1025
2021/11/26 19:45:39 Mail from: foo@bar
2021/11/26 19:45:44 Rcpt to: foo@bar
2021/11/26 19:45:53 Data: Hello, world

client side output:

$ telnet localhost 1025
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
220 localhost ESMTP Service Ready
EHLO test
250-Hello test
250-PIPELINING
250-8BITMIME
250-ENHANCEDSTATUSCODES
250-CHUNKING
250-AUTH PLAIN
250 SIZE 1048576
MAIL FROM: foo@bar
250 2.0.0 Roger, accepting mail from <foo@bar>
RCPT TO: foo@bar
250 2.0.0 I'll make sure <foo@bar> gets this
DATA
354 2.0.0 Go ahead. End your data with <CR><LF>.<CR><LF>
Hello, world
.
250 2.0.0 OK: queued
221 2.4.2 Idle timeout, bye bye
Connection closed by foreign host.

Another case to cause an unexpected behavior: nil the session keeping the c.helo value:

  • upgrade the connection to starttls

    go-smtp/conn.go

    Lines 613 to 616 in 30169ac

    if session := c.Session(); session != nil {
    session.Logout()
    c.SetSession(nil)
    }

  • send MAIL FROM

helo check is passed:

go-smtp/conn.go

Line 297 in 30169ac

if c.helo == "" {

session is nil, but it's method is called without a check for nil value:

go-smtp/conn.go

Line 400 in 30169ac

if err := c.Session().Mail(from, opts); err != nil {

causing panic, e.g. it's reproducible in maddy.

Proposal:

@kayrus
Copy link
Contributor Author

kayrus commented Nov 30, 2021

Some findings in regards to the STARTTLS command from RFC: https://datatracker.ietf.org/doc/html/rfc3207#section-4.2

The server MUST discard any knowledge
obtained from the client, such as the argument to the EHLO command,
which was not obtained from the TLS negotiation itself. The client
MUST discard any knowledge obtained from the server, such as the list
of SMTP service extensions, which was not obtained from the TLS
negotiation itself. The client SHOULD send an EHLO command as the
first command after a successful TLS negotiation.

c.helo must be nulled as well after the STARTTLS succeed.

@emersion emersion added the bug label Dec 21, 2021
@foxcpp
Copy link
Collaborator

foxcpp commented Jan 19, 2022

After #146 changes, there is no way for the backend to signal AUTH being mandatory so its backend responsibility to verify whether AuthPlain was called.

foxcpp added a commit that referenced this issue Jan 19, 2022
@foxcpp
Copy link
Collaborator

foxcpp commented Jan 19, 2022

Since we do not have #147 merged yet, I pushed an intermediate fix for the panic bug.

@emersion emersion removed the bug label Aug 14, 2023
@emersion emersion closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants