Make e-mail sanity check more precise

[balanceofcowards]

For security reasons, all e-mail addresses starting with non-alphanumeric characters were rejected. This is too broad and rejects perfectly valid e-mail addresses. Only leading hyphens should be rejected -- in all other cases e-mail address specification should follow RFC 5322.

[balanceofcowards]

... overlooked the unit tests, sorry.
Having a look at the tests: If e-mail address strings are indeed forwarded to the shell literally, then the dollar ($) and backtick (`) characters should be rejected everywhere in the address, as they could be interpreted in the shell (cf. $HOME@example.com, `pwd`@example.com).

It would be far better to escape user provided strings properly, before handing them to the shell, though. I don't know enough about the exact problem here, though. Please advise.

[lunny]

I think we could just allow - in the non-Cap character of prefix in this PR. Let go of the rope slowly. Maybe there are other security risk we don't know.

[silverwind]

I think we really just should just use mail.ParseAddress, everything else is madness. See #17688 for more context.

[lunny]

I think we really just should just use mail.ParseAddress, everything else is madness. See #17688 for more context.

We can't do that. That's what we changed from and we change it because of some security report about that method.

[silverwind]

We can't do that. That's what we changed from and we change it because of some security report about that method.

Have these "security issues" been raised to golang? Can you provide examples of what was the problem?

[lunny]

We can't do that. That's what we changed from and we change it because of some security report about that method.

Have these "security issues" been raised to golang? Can you provide examples of what was the problem?

See #19102 and #17688

[silverwind]

We ought to just correctly pass -- to sendmail in all cases to fix that. We should not have to restrict emails just because we are using tools like sendmail incorrectly.

[silverwind]

Generally I think we should remove sendmail integration and just do SMTP directly in golang. sendmail is a antiquated tool and there are many different implementations that may not be compatible with each other, it's just a pain in the ass 😉.

[balanceofcowards]

From an external viewpoint, I totally agree with @silverwind. Meanwhile, as a stopgap solution, the sendmail problem can be remediated by checking only for a leading hyphen character.

Regarding my previous remarks about the $ and ` characters: I realized this would only pose a problem if the command was called in a shell context (which is a bad idea, due to exactly this problem). Without knowing details about your code, sendmail likely is not called this way: I tried a basic smoketest on the live test instance and, e.g. a$HOME@domain was processed correctly.

Unfortunately, with the restrictions in place, Gitea is totally broken for me as a user. I cannot access my account and I have no sane way to remediate the problem. I probably could scan the database and replace the offending e-mail address manually... but I would rather not go down that road, fearing what else might break along the way.

[silverwind]

Indeed, we should never run commands in shell context as it's pretty much impossible to escape arguments safely for a shell. We should always use exec.Command(cmd, ...args) format which is safe as long as cmd is not a shell.

[silverwind]

So regarding these changes, they seem okay. But there are various characters that have special meanings in shell context. Can it be demonstrated that this is safe to use with sendmail? Long them, we should of course move away from sendmail to SMTP only.

[codecov-commenter]

Codecov Report

Merging #20991 (8aab3a3) into main (bb0ff77) will increase coverage by 0.48%.
The diff coverage is 62.98%.

@@            Coverage Diff             @@
##             main   #20991      +/-   ##
==========================================
+ Coverage   46.65%   47.14%   +0.48%     
==========================================
  Files        1003     1007       +4     
  Lines      136654   137308     +654     
==========================================
+ Hits        63761    64728     +967     
+ Misses      65066    64661     -405     
- Partials     7827     7919      +92     

Impacted Files	Coverage Δ
cmd/migrate_storage.go	5.88% <0.00%> (+0.11%)	⬆️
models/repo.go	50.22% <0.00%> (+0.11%)	⬆️
modules/avatar/identicon/block.go	42.11% <ø> (ø)
modules/git/foreachref/parser.go	94.36% <ø> (ø)
modules/git/signature_nogogit.go	61.53% <ø> (ø)
modules/log/file.go	68.78% <ø> (ø)
modules/log/smtp.go	82.35% <ø> (ø)
modules/markup/mdstripper/mdstripper.go	61.15% <ø> (ø)
modules/pprof/pprof.go	0.00% <ø> (ø)
modules/structs/issue.go	0.00% <0.00%> (ø)
... and 70 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[balanceofcowards]

So regarding these changes, they seem okay. But there are various characters that have special meanings in shell context. Can it be demonstrated that this is safe to use with sendmail? [...]

In principle, that leads to the same problem @zeripath already outlined in #19102: To be sure, one would have to test every sendmail implementation out there. However, I would consider it unlikely that any sendmail argparser processes special characters other than hyphens.

Regarding characters interpreted by the shell (not sendmail), at first a similar problem would arise: There are different shells out there, which likely have different special characters. The obviously "dangerous" characters would be:

• The dollar sign $ (as $HOME@domain might reveal the home dir of the current process);
• The exclamation mark ! (as !13@domain might reveal command nr. 13 in the history list);
• And the back tick ` (as `pwd`@domain might execute any command).

Note, that these characters do not necessarily depend on being in the first position: $HOME@domain would be as vulnerable as a$HOME@domain. All of these, however, are easily circumvented by not executing external commands in a shell context, in the first place (and I don't think you're doing that, anyway). The second best option would be to escape the characters properly.

In summary, my argument for the PR is: From a security perspective, it does not decrease the status quo, but it fixes the erroneous rejection of valid e-mail addresses for a large number of potential addresses.

[silverwind]

I see we call sendmail with

exec.CommandContext(ctx, setting.MailService.SendmailPath, args...)

Which is save from shell interpretion at least. And anything beyond that are sendmail bugs, so LGTM.

dismiss

[techknowlogick]

ping LG-TM

[cortices]

This is currently a blocking issue in accessing my gitea install due to the incorrect email validation on my admin account. Can this be brought into a release version soon please?

[wxiaoguang]

If it's urgent, to bypass, you can update the admin's e-mail in the database.