Skip to content
This repository was archived by the owner on Mar 29, 2022. It is now read-only.

(WIP) Rewrite common.scrub_filename: meaningful errors, better doc #131

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

awwad
Copy link
Contributor

@awwad awwad commented Sep 11, 2017

common.scrub_filename is used to ensure that vehicle identifiers provided to the Director can be used as directories for the vehicle repositories for those vehicles. A similar function exists in the Primary module, primary.enforce_jail, which is actually security-relevant.

I didn't expect either bit of code (common.scrub_filename) to stick around: I wasn't sure scrub_filename was important (as it's not really a security issue, I don't think), and I expected to firm up enforce_jail (which is a security issue) but after thinking about it, while I think scrub_filename falls under the category of protecting the code from itself and isn't strictly necessary, I think it is still better to retain this than to dispose of it. Unfortunately, primary has very slightly different requirements for primary.enforce_jail, so they can't really be the same piece of code, unless I use an option for excluding '/' or '' characters. /:

enforce_jail matters more because the Primary receives instructions to download filepaths that the Director specifies, and a malicious Director + any compromised Image Repository delegated role might be able to cause the Primary to overwrite arbitrary files in unexpected places, or place new files in unexpected places.

TODO for this PR:

  • Also rewrite enforce_jail and try to consolidate them, perhaps.....

and be more readable and provide a better docstring.
@awwad awwad changed the title Rewrite common.scrub_filename: meaningful errors, better doc (WIP) Rewrite common.scrub_filename: meaningful errors, better doc Sep 11, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 86.003% when pulling df09e55 on awwad:rewrite_scrub_filename into 821464a on uptane:develop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants