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

SendEmail: Protect users against vulnerable logmailers #939

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

philwo
Copy link
Member

@philwo philwo commented Aug 3, 2023

glog is used on a variety of systems, and we must assume that some of them still use vulnerable mailers that have bugs or "interesting features" such as https://nvd.nist.gov/vuln/detail/CVE-2004-2771.

Let's protect users against accidental shell injection by validating the email addresses against a slightly stricter version of the regex used by HTML5 to validate addresses[1].

This should prevent triggering any unexpected behavior in these tools.

Also add some basic unit tests for the SendEmail method.

[1] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

@philwo
Copy link
Member Author

philwo commented Aug 3, 2023

I'm not sure why the AppleClang CI job failed due to some issues with calculating code coverage. If someone could help me fix this, I'd appreciate it.

@ukai
Copy link
Contributor

ukai commented Aug 4, 2023

I'm not sure why the AppleClang CI job failed due to some issues with calculating code coverage.

hmm. using regexp is problematic??

src/logging.cc Outdated Show resolved Hide resolved
src/logging.cc Outdated Show resolved Hide resolved
Copy link
Member

@koto koto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/logging.cc Outdated Show resolved Hide resolved
src/logging.cc Outdated Show resolved Hide resolved
src/logging.cc Outdated Show resolved Hide resolved
@philwo
Copy link
Member Author

philwo commented Aug 18, 2023

I addressed all comments, but the coverage check on macOS still fails. I don't see how my change is related to it, and if I run the lcov commands locally on my Mac with latest Xcode they fail at even more places.

Is this just broken in general at the moment due to latest Xcode being stricter / buggy?

@philwo
Copy link
Member Author

philwo commented Aug 22, 2023

@sergiud Do you have some advice what to do about the code coverage errors?

@sergiud
Copy link
Collaborator

sergiud commented Sep 4, 2023

Thanks for bringing the issue to my attention. Let me look into it.

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ignore the coverage problem as it is a separate issue.

src/logging.cc Show resolved Hide resolved
glog is used on a variety of systems, and we must assume that some of
them still use vulnerable mailers that have bugs or "interesting
features" such as https://nvd.nist.gov/vuln/detail/CVE-2004-2771.

Let's protect users against accidental shell injection by validating
the email addresses against a slightly stricter version of the regex
used by HTML5 to validate addresses[1].

This should prevent triggering any unexpected behavior in these tools.

Also add some basic unit tests for the SendEmail method.

[1] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
@philwo philwo force-pushed the philwo-mailvalidation branch from ecb449d to 0bad5a5 Compare September 7, 2023 11:11
@philwo
Copy link
Member Author

philwo commented Sep 7, 2023

Seems like we're good? You mentioned that the coverage failure on Apple is a separate issue, and the Windows clang build failure on Bazel CI also happens on the master branch, so seems unrelated. 😊

@philwo
Copy link
Member Author

philwo commented Sep 7, 2023

My friends in the Bazel team said that the Windows clang failure should be fixed in Bazel 6.4.0 LTS: bazelbuild/bazel#19430

@sergiud
Copy link
Collaborator

sergiud commented Sep 7, 2023

Thanks! This is good to go now!

@sergiud sergiud added this to the 0.7 milestone Sep 7, 2023
@sergiud sergiud merged commit 6482757 into google:master Sep 7, 2023
104 of 107 checks passed
@philwo philwo deleted the philwo-mailvalidation branch September 12, 2023 02:34
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.

4 participants