Misuse of commit permissions

1

Over on
keepassxc: Enable networking by default by talyz · Pull Request #103146 · NixOS/nixpkgs · GitHub I had a dicussion with @talyz about how one of his PRs was a security risks. And yet he has gone ahead and merged it, despite no approval from others.

Not only was this PR bad in itself, his argument boils down to - just because there already exists an attack surface, somehow it’s okay to increase it.

This kind of arbitrary self merge of non-urgent PRs(which imho still should have a post merge approval or revert policy but beside the point of this thread) will erode trust in the security of NixOS.

I raised my objection over on the PR too but I suspect that the author made up his mind as soon as he submitted his PR and the arguments were all decided afterwards, and as such a good faith discussion is no longer possible.

2

Not only was this PR bad in itself, his argument boils down to - just because there already exists an attack surface, somehow it’s okay to increase it.

I think you should not have ommitted in your summary that upstream enables the option that the PR enables, not just other distributions some of which actually have more resources to determine the security stance than NixOS.

5 Likes
3

I should have given a heads-up before merging and I’m sorry I didn’t.

And yet he has gone ahead and merged it, despite no approval from others.

While it’s true that there were no PR approvals, support was voiced for the HIBP integration.

Not only was this PR bad in itself

Unless you describe in what way it was bad, this only serves as an attack.

his argument boils down to - just because there already exists an attack surface, somehow it’s okay to increase it.

Not really. Here is the quote that I’m guessing you’re referring to:

Lastly, I feel like you’re worrying way too much over a feature that could theoretically be used as an attack vector when there’s a much more obvious one already enabled - the browser integration. The way the favicon fetching works, it’s also entirely optional, even when enabled at compile time; you have to press a dedicated fetch favicon button for it to actually do it.

What I mean by that is that you have to take into consideration the size of the attack surface as a whole: if we already have a big attack surface due to enabling the browser integration, increasing it slightly won’t make a big difference to the security of most users. If we want to harden the KeePassXC, we should focus on disabling all potential attack surfaces either in the main package or, preferably, a secondary one. But, as I’ve made quite clear at this point, I don’t think this is necessary, since the security risks should be almost entirely dependent on the user actually making use of them: KeePassXC won’t automatically fetch favicons, you have to click a dedicated button for it to do this, and the browser integration has to be manually enabled in the settings. Looking at the real-world impact, it might even be that the HIBP integration has a greater positive impact on user security than the negative impact of favicon fetching, yielding a greater attack surface, yet increased user security.

4 Likes
4

Looks good to me. If this is important to you @SRGOM then you can also add a -hardened version to the package set.

Dealing with differing opinions is tricky. Especially since we are spread so thin over many PRs. If in doubt, deferring to upstream seems like a sane default position to have. Another thing to consider is that it’s demotivating to only get negative feedback without an alternate proposal. As much as possible we should try to provide a road forward.

But I also understand the concern. Finding the right solution is a balancing act and we are not always going to get it right, or even be consistent between so many reviewers.

11 Likes
5

@7c6f434c

I already gave the link of PR and the discussion is easy to follow.

Secondly, I have lready discusses on the iusse tracker how I don’t think that it’s appropriate to defer to someone else and the arguments need to be made from first principles. For all know you, someone made a PR in upstream, there were objections , and ultimately it was self merged because the PR submitter had no respect for the process. Even otherwise, how do we know it was a security minded invidiual and not a popularity minded individual?

@talyz I did not reply to your last post because it was clear you have no clue what you are talking about. (As you yourself explicitly admitted- you have neither the knowlege of code nor you are confident of it, you mentioned that upstream understands security better than “us”). That said, for your information in future, just because there is a term “browser” in therr doesn’t mean it’s insecure. There is a proper channel over which text request for password entry is sent and received (hopefully in a fix sizes buffer), all the db handling is on keepass side. Contrast this with binary resource parsing which is huge and filled with landmines. (Not that I would ever recommend browser integration to anybody)

This discussion further emphasises why this PR is rubbish- every tom dick and harry knows the risk associated with repeating passwords or using low entropy passwords (at least a nix using TDH), few know how dangerous it can be to try to parse binary resources (and thus an extra button still puts people at risk)

I have already argued against your HIBP point over on GitHub, and bringing HIBP afterwards (when interestingly your original PR had no mention) further shows you’re being facetious.

@zimbatm
My objection to PR code was on GitHub, my objection here is the absolute mockery of process.

I appreciate your point of view, this is volunteer work and contributions need to be respected,believe me I know i hate nitpicks in PR. I have myself mentioned that there should be lower standards for volunteer contributions.

But one needs to draw a line somewhere and take ownership about what is sensitive code.

There is a very simple way forward here, maintain the status quo. The feature adds bare minimal but has major security implications. Being extremely paranoid with password managers is absolutely the single narrow rational path.

As for whether it’s right to expect commiters to be expert on everything- I agree that deferring to upstream a very logical way, ONLY as long as there are no first principle objections (and I already raised them).

6

For all know you, someone made a PR in upstream, there were objections , and ultimately it was self merged because the PR submitter had no respect for the process. Even otherwise, how do we know it was a security minded invidiual and not a popularity minded individual?

If you believe this about KeePass, then you should only use it if you have a very clear model of how development compartmentalisation happens there.

7

I use it in local mode- no talking to outside world. I use it because the keepass cryptography decisions, to my knowledge, are audited.

Once again i would request you argue from first principles and not defer to upstream.

Look I’m not saying I know everything that is right or wrong. All I’m saying is- here is my argument. And if you reply …“but but this one person I know who does things this way…” Then that’s improper for the project and disrespectful to me. I very much welcome a good faith counter argument from you.

8

Also- for the record, since there are people with commit permissions who use it to arbitrarily commit sensitive code and at the same time think that somehow I’m thinking too much- Microsoft Security Bulletin MS15-024 - Important | Microsoft Learn

9

Please be polite. Personal attacks do not benefit any discussions. If you believe they are incorrect, explain where there reasoning is wrong. Or if you do not have the time or energy, just leave it where it is.

Is there a process? In this case the change was in correspondence with upstream and one of the package maintainers indicated that they do not have an issue with this change. I think your criticism is valid, but people have different threat models and security/convenience trade-offs.

I agree with @zimbatm that there is a nice technical solution that sidesteps these differing priorities. Either add an extra attribute for the hardened version or make the hardened version the default and add a Full attribute that has such features enabled.

8 Likes
10

Could there just be a rule, you don’t get to commit your own PR’s?

That way at least 2 people are involved in whatever happens.

This is what we do at $WORK, and seems to fix a lot of issues(and makes our auditor happy). We don’t use github, so I don’t know how feasible it is to have Github enforce it. It might be OK to leave it un-enforced until it becomes an actual problem.

4 Likes
11

Could there just be a rule, you don’t get to commit your own PR’s?

That way at least 2 people are involved in whatever happens.

This is what we do at $WORK, and seems to fix a lot of issues(and makes our auditor happy). We don’t use github, so I don’t know how feasible it is to have Github enforce it. It might be OK to leave it un-enforced until it becomes an actual problem.

  1. It is completely impossible to enforce if you want to welcome new contributors that just tried NixOS and created a GitHub account to submit a patch to solve their personal annoyance.

  2. It will make the problem of PR backlog worse.

(and yeah, gets proposed from time to time)

(and yeah, wouldn’t help here, because we see an obvious split between people who consider deferring to upstream good enough for a default behaviour, and others)

4 Likes
12

I think they meant to enforce a rule that people in nixpkgs-committers cannot merge their own PRs to master.

I do like this principle, but I think in nixpkgs this would result in many more stalled PRs. There are a lot of version bumps that are safe to merge. And I’d rather merge them after 1-2 weeks if they don’t get reviews than have them stuck forever.

1 Like
13

I think they meant to enforce a rule that people in nixpkgs-committers cannot merge their own PRs to master.

You can establish a policy like that, but not enforce it against willful violations because sockpuppets and newcomers are indistinguishable.

I do like this principle, but I think in nixpkgs this would result in many more stalled PRs.

The risk is also competing for attention with non-committer contributor PRs.

2 Likes
14

Thanks for the clarification, that point ‘wooshed’ past me.

1 Like
15

Yes, but you also misrepresent it in your original post, both by cherry-picking and making incorrect assumptions.

Not sure what to make of this - you ignore the discussion in the PR and then complain that you were the one being ignored?

Why would you expect me to be familiar with the KeePassXC codebase? Never ever have I seen that being expected for any similar change in nixpkgs or elsewhere. I sincerely hope that upstream have a better understanding of the security pertaining to KeePassXC since they’re the ones making it and we’re just packaging it.

No, it’s a potential attack vector because it provides an API for fetching content from the db and usually connects to a browser, which does much more than parsing jpegs and pngs.

This is certainly not true - not everybody using nix knows knows all of its intricacies or even what nix is, since it can for example be used by operations to deploy software on client machines (on macs in particular). We do this at my company and I know of others who do it too. Also, some old passwords may have been imported without updating them, some may be shared and can’t be updated as easily, etc.

Sure, theoretically a successful attack could give arbitrary code execution and allow the attacker to exfiltrate sensitive data, but exactly how easy or feasible do you think such an attack really is? If it’s easy to pull off, that means almost all KeePassXC users are at risk and the feature should likely be removed upstream.

You wrote the following

I agree about HIBP and it’s arguably useful enough to justify inclusion. That said, we need to consider the average keepass user, and even there the average NixOS user, and there’s a decent chance that all passwords are unique and high entropy.

where the first part is positive and I’ve countered the second part. However, I’m mostly referring to jonafato’s comment on HIBP:

I don’t have strong feelings on the default value of this flag. If it’s going to flip, HIBP integration is probably the best reason to do so, as it provides some tangible security benefit to users.

I learned of the HIBP integration a few hours after submitting the PR; not sure why that’s relevant, though.

16

on 1) if a rule like this existed, you are correct it’s hard to determine if a new person is just a “sockpuppet” and created the account just to bypass the rule, but 1) they would have to be a committer already 2) if/when they are caught, you probably wouldn’t want them to be committers any longer and take their commit bit away. That’s just underhanded terribleness(assuming the rule was in place).

  1. It might make the PR backlog problem worse, there certainly is a case that it could. But if you pair committers, then it might not.

I think version bumps are 99% of the time always safe to merge, so maybe there is an exception here, but even without an exception, it’s a very trivial PR, so it’s easy to see what it’s doing and merge, even if you aren’t the contributor.

Also, just because you can’t commit your own work, as a rule, doesn’t imply that you have to do a thorough code review before committing.

For this particular case, it would at least confirm some other committer thought it was fine to merge as is, despite the comments in the PR request.

I am definitely new to NixOS, and haven’t seen it proposed before, I apologize. It could at least be discouraged that you commit your own PR’s. (which amounts to a rule that’s not enforced via github/etc)

17

At some point, we will get to a point where we have enough automation to enforce these rules more consistently. There was a RFC to disallow direct pushes to master for example. I think that the desire is there and it’s only a matter of time.

One point that is missing from the discussion (or sorry if I missed it, this thread is getting quite long), is that maintainers of a package should be getting more precedence on decision making. As a maintainer, I want to be able to bump my package and merge it.

It’s not an official rule, but to me, if there is an argument, the maintainer is the one that does most of the work so they get to split the decision.

7 Likes
18

Guys, what we need here is a process for conflict resolution which we have talked about before in a different context - I’m referencing an earlier comment I made on the point: How many people are paid to work on Nix/Nixpkgs? - #11 by peterhoeg

I’ll come up with an RFC within the next 2 weeks and I hope the participants in this thread will all chime in.

5 Likes
19
  1. It might make the PR backlog problem worse, there certainly is a case that it could. But if you pair committers, then it might not.

How can it not make the problem worse if now people need to click merge on more things?

Sure, it can train people to not read what they merge at all, there is a small risk of that.

I am definitely new to NixOS, and haven’t seen it proposed before, I apologize. It could at least be discouraged that you commit your own PR’s. (which amounts to a rule that’s not enforced via github/etc)

You are most surely not at fault, there is too much going on to keep up even without being new. Just wanted to say it is not a topic that goes for long without a discussion.

1 Like
20

At some point, we will get to a point where we have enough automation to enforce these rules more consistently. There was a RFC to disallow direct pushes to master for example. I think that the desire is there and it’s only a matter of time.

The pushback is also here, and will not go away anytime soon.

And no-direct-pushes is just about how far the tooling around the CI is from the point where pushing everything through CI is not more annoyance than benefit.

And note that not all of the people who want to make discourage self-merges favour the policies like rapid systematisation and increase in commit access handout, so it’s not like improvements in tooling will automatically solve the remaining arguments.