Tails

#4 Updated by intrigeri 2019-07-05 10:02:18

So this is about:

• integrity of tails.git apart of website PO files, i.e. ensuring that the security checks performed on Git pushes by Weblate are good enough and effectively enforced
  • Enrico already did a security-oriented code review of the hook so I won’t spend much time on it
  • I’ll focus on the big picture and integration bits, where potential design/implementation flaws could allow bypassing the aforementioned security checks
• availability of the infrastructure outside of the translation platform, that is impacted by the translation platform, that is in practice: can the translation platform DoS anything except itself? Attack vectors: validating arbitrary translated strings, compromising the Weblate webapp, compromising the system of a Tails sysadmin, compromising the system of an administrator of the translation platform (that can run any command as the weblate user).
• integrity of our production website, which in our permissions model boils down to ensuring that translated strings pushed by Weblate have gone through the expected review process (it is the only mitigation we have in place against this risk)

Out of scope:

• An adversary acting as several translators can push arbitrary changes to translated versions of our web pages. Impact can be, for example: instructing our users to take dangerous actions; hiding important security warnings; breaking page display or links in a way that makes the doc hard or impossible to follow. This risk comes directly from trade-offs we consciously made while designing our translation platform so we have to live with it.

#6 Updated by intrigeri 2019-07-05 11:15:24

intrigeri wrote:
> * Where is the expected review process documented and implemented?
> * What’s the (technical, social) process to gain elevated credentials not available to anyone who registers an account?
>
> Once I am pointed in the right direction, I’ll check what it currently takes an adversary (possibly posing as several individual translators) to suggest arbitrary translations and validate them so they land on our website.

The thread that starts in https://lists.autistici.org/message/20190627.121709.5f198b23.en.html is about basically the same questions.

#9 Updated by intrigeri 2019-07-05 22:05:58

intrigeri wrote:
> recommendations:
> * optimize the reModifyFile regexp to limit the need for backtracking; it is probably acceptable, when doing so, to make the regexp a tiny bit less strict, e.g. currently it requires page names to be at least 3 chars long (ABC.LL.po) and to not end with a dot, which is not needed AFAICT

hefee suggested I help with this regexp so I’ve tried to understand what criteria it expresses. I see it was modified from (essentially) .*\.po$ to what we now have in f8a455c0c543c43113fb5023dd6432fda1a2b4ba, which is not exactly atomic and whose message reads “Integrate enricos review for tails-weblate-update.hook”. This lack of explanations (or pointers to them) makes it hard for me to tell whether it’s safe to remove some of the restrictions added in that commit while optimizing this regexp: I have no idea what the reasoning behind these restrictions was. So instead I’ve looked closer and it turns out that this regexp should not expose us to the most pathological cases of backtracking, so nevermind, feel free to forget this recommendation.

Still, I’d feel more comfortable if the reasons for these restrictions (e.g. “path does not start with a dot” and “need a non-dot char between .LL.po”) were documented as comments in the code, so that whoever works on it again in the future is in a better position to make sensible decisions. I’m mostly expecting ways to distinguish between “can’t hurt, better safe than sorry, but not strictly needed at the moment” and “truly critical right now”. See what I mean?

Finally, I would like to add a comment about what kind of path the regexp is used on but I need a confirmation here: fname = diff.b_path, that we call the regexp on, is the full path of a modified file, right? That is, in normal cases it will start with wiki/src/, correct? (If all this is correct, then I was wrong earlier: thanks to this there’s actually no “page names to be at least 3 chars long (ABC.LL.po)” restriction currently. I got confused initially, which is why I’d like to document this, at least for the sake of some future version of /me :)

#11 Updated by hefee 2019-07-06 21:24:27

• optimize the reModifyFile regexp to limit the need for backtracking; it is probably acceptable, when doing so, to make the regexp a tiny bit less strict, e.g. currently it requires page names to be at least 3 chars long (ABC.LL.po) and to not end with a dot, which is not needed AFAICT

What do you think about gettting rid of the regex completely and make it more easy to understand:

<code class="python">
if not fname.endswith(".po") or fname.split["/"][-1].startswith("."):
   issues.append("[POLICY] Weblate is only allowed to modify po files.")
if not any(fname.endswith(".{}.po".format(i) for i in ALLOWED_LANGUAGES):
   issues.append("[POLICY] Language not allowed to push.")
</code>

(any creates a generator, so if one matches, it stops right away, and we do not loose CPU cycles)

• if feasible, use iterators instead of loading rev_list and oldHead.diff(newHead) in memory all at once

- rev_list is already a generator (that is the python term for iterators) - nothing to do here.

- oldHead.diff, there is no generator version available - PythonGit needs to ask git binary in background, I don’t see what we can improve here. Keep in mind, that by default the diff objects only holds the path and metadata of the diff, not the complete diff itself. Only if you do oldHead.diff(newHead, create_path=True), than this object holds the concrete diff. That’s why I think also for several thousands of files, it should be not become an issue that fast in terms of memory.

#12 Updated by intrigeri 2019-07-09 13:44:47

> What do you think about gettting rid of the regex completely and make it more easy to understand:

Well, similarly to what I’ve discussed above, I’m not comfortable removing checks without understanding why they were added in the first place, so I’m not in a position to comment in an informed manner here.

This being said, it’s probably better to you’re proposing and document in the commit message why the checks were added and why we don’t actually need them than to do what I’ve requested (documenting the reason for the checks in a comment): in both cases, one needs to look into the review / MR history and describe exactly the same facts. So yeah, go ahead :)

> * if feasible, use iterators instead of loading rev_list and oldHead.diff(newHead) in memory all at once

> - rev_list is already a generator (that is the python term for iterators) - nothing to do here.
> - oldHead.diff, there is no generator version available

OK!

#13 Updated by intrigeri 2019-07-09 14:03:04

> This being said, it’s probably better to you’re proposing and document in the commit message why the checks were added and why we don’t actually need them

I meant “it’s probably better to do”.

#15 Updated by intrigeri 2019-07-12 19:25:59

> I replaced the reModifyFile with a version that holds our expectations and made the tests run on Python 3.5.

Merged.

(I’m not closing this ticket as we still need the reviewer policy.)