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

Fix umask handling when creating new files #58

Closed
wants to merge 1 commit into from

Conversation

domcleal
Copy link
Member

@domcleal domcleal commented Dec 2, 2013

  • src/transform.c (transform_save): faulty umask arithmetic would cause
    overly-open file modes when the umask contains "7", as the umask was
    incorrectly subtracted from the target file mode

Fixes CVE-2013-6412, RHBZ#1034261

https://bugzilla.redhat.com/show_bug.cgi?id=1034261 for reference.

  * src/transform.c (transform_save): faulty umask arithmetic would cause
    overly-open file modes when the umask contains "7", as the umask was
    incorrectly subtracted from the target file mode

Fixes CVE-2013-6412, RHBZ#1034261
@domcleal
Copy link
Member Author

domcleal commented Dec 9, 2013

ping @lutter, could you review please?

@dev-zzo
Copy link

dev-zzo commented Dec 11, 2013

Apologies for butting in. I'm just wondering, if you are able to set umask to arbitrary values (this seems to be the attack vector, right?), you can set it to 000 -- the result is still the same (0666 perms for the created file)? Might be a problem if the original file has 0600 or similar.

Does this patch really fix the issue in question?

@domcleal
Copy link
Member Author

@dev-zzo thanks for the feedback, you're not butting in at all. The more eyes on this code, the better!

The problem is simply that a umask of say "027" results in permissions of "-rw--wxrwx" due to a bad umask calculation, when it should be resulting in a file mode of 0640.

If the user sets their umask to 000, then it should create a file with 0666 permissions as that's what they've requested. If the original file exists though and has 0600 permissions, the code takes a different path and copies the file attributes from the original:

https://github.com/domcleal/augeas/blob/f4f9fa61c0e0e5a10e19e0f48df31022e842dfcc/src/transform.c#L1136-L1140

Note at this point we're creating a temporary file, which always starts with secure permissions (0600) and then we loosen them to match either the original file or the umask.

@dev-zzo
Copy link

dev-zzo commented Dec 11, 2013

Thanks a lot for the clarification! Appreciate it.

@lutter
Copy link
Member

lutter commented Jan 14, 2014

Great catch and good patch. Committed in f5b4fc0

@lutter lutter closed this Jan 14, 2014
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.

None yet

3 participants