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

Fold long lines during smtp communication in Protocol\Smtp class #138

Closed
wants to merge 2 commits into from

Conversation

vladsf
Copy link
Contributor

@vladsf vladsf commented Feb 26, 2021

Q A
Documentation no
Bugfix yes
BC Break ??
New Feature no
RFC yes
QA no

Description

Fold long lines following RFC 5322 section-2.2.3

To deal with the 998/78 character limitations per line,
long lines can be split into a multiple-line representation
separated by CRLF + SPACE; this is called "folding".
Correct folding is particularly important for long header fields
(e.g. generated by MS Exchange).

@glensc adds:
It seems, the underlying problem of the bug is that currently, the loop reads up to 1000 bytes, and if the input is longer than that, then everything over 1000 bytes will appear as the next line in mail headers, resulting in corruption. And depending on MTA, such corruption is treated as the end of headers input.

To deal with the 998/78 character limitations per line,
long lines can be split into a multiple-line representation
separated by CRLF + WSP; this is called "folding".
Correct folding is particularly important for long header fields.

Signed-off-by: Vlad Safronov <[email protected]>
Signed-off-by: Vlad Safronov <[email protected]>
@glensc
Copy link
Contributor

glensc commented Feb 26, 2021

@vladsf could you cross-link with the discussion in Eventum thread where this was discussed, please.

@glensc
Copy link
Contributor

glensc commented Feb 26, 2021

Maintainers: looks like the RFC in PR template is ambiguous, does it mean generic Request for Comments aka discussion, or actual some relation to official RFC index, i.e https://tools.ietf.org/html/rfc5322

I think it's better to avoid the acronym and write out Discussion, or remove the option at all?

@glensc
Copy link
Contributor

glensc commented Feb 26, 2021

@vladsf could you update PR title that this affects only Protocol\Smtp class.

@vladsf vladsf changed the title Fold long lines following RFC 5322 section-2.2.3 Fold long lines during smtp communication in Protocol\Smtp class Feb 26, 2021
@glensc
Copy link
Contributor

glensc commented Feb 27, 2021

@vladsf:

It seems, the underlying problem of the bug is that currently, the loop reads up to 1000 bytes, and if the input is longer than that, then everything over 1000 bytes will appear as the next line in mail headers, resulting in corruption. And depending on MTA, such corruption is treated as the end of headers input.

could you copy this explanation to PR body please.

@glensc
Copy link
Contributor

glensc commented Feb 27, 2021

@vladsf your solution postpones the problem when the next limit, PHP_SOCK_CHUNK_SIZE (8192) is hit.

more appropriate would be to check if the string is not read up to "\n" (last byte being the terminator) and read again;

UPDATE:
seems stream_get_line used here is behaving differently, the trailing "\n" is removed from returned value:

so, the other way to detect if the line read was complete is to compare if read length equals to buffer_length-1. But that leaves in the possibility that input was exactly buffer_length long. And in that case, the two unrelated headers would be joined as the same header...

so, maybe switch to use fgets, as its API is more predictable.

maybe maintainers can chime in here and give directions?

@glensc
Copy link
Contributor

glensc commented Feb 27, 2021

UPDATE, I took my own note and converted it into commits #140

so, perhaps this one can be closed as a duplicate.

@vladsf
Copy link
Contributor Author

vladsf commented Feb 27, 2021

OK, let me look into #140.

@vladsf vladsf closed this Feb 27, 2021
@vladsf
Copy link
Contributor Author

vladsf commented Feb 27, 2021

Closed as a duplicate of #140.

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

Successfully merging this pull request may close these issues.

2 participants