1250769 - Add a hook to stop people from pushing commits with message highlights security problems

Status: REOPENED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

I was educated by Waldo that I should not have landed some commit which mentions keywords highlight security fix. I feel sorry about that, but I think nobody told me that before... And I don't even know that commit has security concerns...

Thus, instead of educating people over and over again, I suggest we have a server-side hook to stop people from publishing any commit with sensitive message to the public with a message pointing to some guideline for security bugs, so that developers like me won't expose security issue accidently.

Security guys could work out a detailed list of keywords we should detect.

Comment 1

[Gregory Szorc [:gps]]

I'm going to say this is a dupe of bug 1158178.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

No, I don't think it is about the sec-approval.

Security-sensitive keywords should be denied for almost all cases, with or without sec-approval. Also pushing a commit with sensitive keyword publicly could happen before the corresponding bug is turned into security bug, e.g. try push and mozreview push.

Comment 3

[Jeff Walden [:Waldo]]

This would be to forbid a push with a commit message like, "Bug 123456 - Fix arbitrary code execution vulnerability in the security widget.  r=sparky", where more circumspection is desirable.  This doesn't require any checking with third-party servers or anything, just a blacklist of dubious terms suggesting the commit message should be rewritten to not be so direct about it.

Also, this could totally be populated with a wholly-bogus list of such terms/regular expressions to start -- no need to delay implementation, or even deployment, to iron out an exact list.

Comment 4

[Gregory Szorc [:gps]]

I'm not enthusiastic about this request. First, sanitizing commit messages is security through obscurity, which isn't real security. Second, I don't like hooks that can be accidentally triggered and add process overhead. This request feels like it will trigger a number of false positives. e.g. a commit message could contain "we do X because it could lead to arbitrary code execution." Third, the hook can give a false sense of security. It won't catch everything. So there will be a cat and mouse game to catch more and more phrases. This will increase false positive rate.

How often do people push commits that inadvertently draw attention to security issues? Is it frequent enough to justify the overhead from false positives and the false sense of security the hook will give?

I'm not convinced there is a serious enough problem that warrants addressing here.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Gregory Szorc [:gps] from comment #4)
> I'm not enthusiastic about this request. First, sanitizing commit messages
> is security through obscurity, which isn't real security.

It's not encouraging people to simply make message obscure. We want to push people to reconsider the bug they are fixing, and probably mark the bug security-related, and follow the security process.

OTOH, obscurity could somehow secure us, as it makes it harder for people to find those kind of issue via simply greping our commit log.

Note that, there is one question in our security approval request that:
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
This clearly emphasizes the importance of the obscurity itself.

Comment 6

[Gregory Szorc [:gps]]

This isn't a priority for me. But if someone writes code for identifying security sensitive messages, feeds it through the commit messages for Firefox and proves its usefulness, we can consider deploying it as an extension.

If you want to hack something together, I'd write a Python script that consumes the output of `hg log -Tjson` or `hg log -T '{desc}\0', feeds the commit messages through whatever parser you write, and prints results. The hard part here is writing the function that analyzes commit messages. If you can provide that code, it is trivial to turn into a Mercurial hook.