LWPCookieJar.save() creates *.lwp file in 644 mode #79096

aleskva · 2018-10-06T13:58:57Z

BPO	34915
Nosy	@vadmium, @tirkarthi

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2018-10-06.13:58:56.935>
labels = ['type-security', '3.8', '3.9', 'expert-IO', '3.7', 'library', '3.10']
title = 'LWPCookieJar.save() creates *.lwp file in 644 mode'
updated_at = <Date 2021-03-14.06:38:53.953>
user = 'https://bugs.python.org/aleskva'

bugs.python.org fields:

activity = <Date 2021-03-14.06:38:53.953>
actor = 'martin.panter'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'IO']
creation = <Date 2018-10-06.13:58:56.935>
creator = 'aleskva'
dependencies = []
files = []
hgrepos = []
issue_num = 34915
keywords = []
message_count = 4.0
messages = ['327246', '339126', '341422', '388664']
nosy_count = 3.0
nosy_names = ['martin.panter', 'aleskva', 'xtreak']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue34915'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

aleskva · 2018-10-06T13:58:57Z

The LWPCookieJar.save() creates an *.lwp file containing session cookies in non-safe 644 mode (everyone can read it). This is not a secure behavior, especially for storing session keys or session cookies. The file should be created in 600 mode in my opinion.

https://github.com/python/cpython/blob/3.7/Lib/http/cookiejar.py#L1872

tirkarthi · 2019-03-29T17:05:40Z

I guess this is a good choice and distutils stores .pypirc [0] in this manner that has username and password.

[0]

cpython/Lib/distutils/config.py

Line 45 in 2f54908

with os.fdopen(os.open(rc, os.O_CREAT | os.O_WRONLY, 0o600), 'w') as f:

tirkarthi · 2019-05-05T06:41:05Z

Martin, any thoughts on this change?

vadmium · 2021-03-14T06:38:53Z

I don't have a strong opinion, but it does seem a sensible change that matches the high-level nature of the "cookiejar" module, with low risk of users relying on the current file permissions. On the other hand, the "curl" command seems to use the default mode when creating a cookies file (in Netscape a.k.a. Mozilla format):

$ curl --cookie-jar cookies https://www.google.com/
[. . .]
$ ls -l cookies
-rw-r--r-- 1 vadmium vadmium 418 Mar 14 17:12 cookies

The MozillaCookieJar class also seems to use the default file mode. I suppose it should be changed as well as the LWP class.

Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue python#79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place.

….save() Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue python#79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place.

….save() Note: This change is not effective on Microsoft Windows. Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue python#79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place.

ambv · 2022-06-07T07:48:51Z

FWIW Firefox also creates user profile directories with 700 so that looks like precedent to me.

ambv · 2022-06-07T07:53:56Z

@AA-Turner removed the security branches from this issue suggesting that this isn't a security vulnerability in need of patching. Following this train of thought, , I'm not entirely sold on the idea that this change is even a "bug fix". It looks more like an enhancement to me, which I would err on especially given its backwards incompatibility. It's @pablogsal's call, I would consider it for 3.11 but not for 3.10.

@tiran's got more experience with evaluating security issues so he can also weigh in on what we should do here.

GH-93463) Note: This change is not effective on Microsoft Windows. Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue #79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place.

ambv · 2022-06-07T08:11:50Z

I merged gh-93463 to 3.12. Waiting on @pablogsal and/or @tiran to voice their opinion on what to do with backports here.

tiran · 2022-06-07T08:24:43Z

I share @ambv concerns regarding backward incompatibility. It's a potential security issue iff an application also uses insecure directories to store the cookies. Any security sensitive application should use directories with tight permissions for sensitive data. A strict umask like 0o027 or 0o077 would also create cookie jars that are not accessible to users.

Could you please improve the test code?

remove print
use os_helper.unlink() instead of try/except block
use stat.S_IMODE(status.st_mode) == 0o600 instead of oct()[-3:]

AA-Turner · 2022-06-07T08:29:35Z

@AA-Turner removed the security branches from this issue suggesting that this isn't a security vulnerability in need of patching.

Sorry, I should have added a comment -- to confirm, my thinking was along the same lines as yours.

A

ambv · 2022-06-07T08:31:12Z

Oops, since I already landed gh-93463 in 3.12, I'll do the changes @tiran wants in a separate PR.

tiran · 2022-06-07T09:47:57Z

@ambv Could you please add @os_helper.skip_unless_working_chmod decorator, too? It's a new decorator for testing on wasm32-wasi.

Fixup of pythonGH-93463: - remove stray print - use proper way to check file mode - add working chmod decorator

Fixup of GH-93463: - remove stray print - use proper way to check file mode - add working chmod decorator Co-authored-by: Łukasz Langa <lukasz@langa.pl>

pSub · 2022-06-09T12:29:58Z

Thank you for your review @tiran, I learned a lot. And thank you @ambv for addressing the review findings.

ambv · 2022-06-09T13:31:00Z

@pablogsal, let us know if you'd agree for this to still land in 3.11. Otherwise this can be closed.

pablogsal · 2022-06-09T13:32:58Z

I'm fine landing this in 3.11 as long as it goes in before next beta

ambv · 2022-06-09T13:33:41Z

Alright, I'll do the backport now in one commit.

….save() (pythonGH-93463) Note: This change is not effective on Microsoft Windows. Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue python#79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place.

Fixup of pythonGH-93463: - remove stray print - use proper way to check file mode - add working chmod decorator Co-authored-by: Łukasz Langa <lukasz@langa.pl>

…r.save() (GH-93463) (GH-93636) Note: This change is not effective on Microsoft Windows. Cookies can store sensitive information and should therefore be protected against unauthorized third parties. This is also described in issue #79096. The filesystem permissions are currently set to 644, everyone can read the file. This commit changes the permissions to 600, only the creater of the file can read and modify it. This improves security, because it reduces the attack surface. Now the attacker needs control of the user that created the cookie or a ways to circumvent the filesystems permissions. This change is backwards incompatible. Systems that rely on world-readable cookies will breake. However, one could argue that those are misconfigured in the first place. Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Pascal Wittmann <mail@pascal-wittmann.de> Co-authored-by: Christian Heimes <christian@python.org>

ambv · 2022-06-09T14:18:36Z

Alright, this is landed to 3.11 and 3.12. Thanks, @pSub! ✨ 🍰 ✨

aleskva mannequin assigned tiran Oct 6, 2018

aleskva mannequin added expert-SSL 3.7 stdlib expert-IO type-security labels Oct 6, 2018

tiran added 3.8 3.9 3.10 labels Oct 21, 2020

tiran removed their assignment Oct 21, 2020

vadmium removed the expert-SSL label Mar 14, 2021

ezio-melotti transferred this issue from another repository Apr 10, 2022

pSub mentioned this issue Jun 3, 2022

gh-79096: Protect cookie file created by {LWP,Mozilla}CookieJar.save() #93463

Merged

AA-Turner added 3.11 3.12 and removed 3.8 3.7 labels Jun 3, 2022

ambv removed the 3.9 label Jun 7, 2022

tiran added a commit to tiran/cpython that referenced this issue Jun 8, 2022

pythongh-79096: Fix/improve http cookiejar tests

f914ac4

Fixup of pythonGH-93463: - remove stray print - use proper way to check file mode - add working chmod decorator

ambv added a commit that referenced this issue Jun 8, 2022

gh-79096: Fix/improve http cookiejar tests (GH-93614)

68f6a5d

Fixup of GH-93463: - remove stray print - use proper way to check file mode - add working chmod decorator Co-authored-by: Łukasz Langa <lukasz@langa.pl>

ambv mentioned this issue Jun 9, 2022

[3.11] gh-79096: Protect cookie file created by {LWP,Mozilla}CookieJar.save() (GH-93463) #93636

Merged

ambv closed this as completed Jun 9, 2022

LWPCookieJar.save() creates *.lwp file in 644 mode #79096

LWPCookieJar.save() creates *.lwp file in 644 mode #79096

aleskva mannequin commented Oct 6, 2018

aleskva mannequin commented Oct 6, 2018

tirkarthi commented Mar 29, 2019

tirkarthi commented May 5, 2019

vadmium commented Mar 14, 2021

ambv commented Jun 7, 2022

ambv commented Jun 7, 2022

ambv commented Jun 7, 2022

tiran commented Jun 7, 2022

AA-Turner commented Jun 7, 2022

ambv commented Jun 7, 2022

tiran commented Jun 7, 2022

pSub commented Jun 9, 2022

ambv commented Jun 9, 2022

pablogsal commented Jun 9, 2022 •

edited

ambv commented Jun 9, 2022

ambv commented Jun 9, 2022

LWPCookieJar.save() creates *.lwp file in 644 mode #79096

LWPCookieJar.save() creates *.lwp file in 644 mode #79096

Comments

aleskva mannequin commented Oct 6, 2018

aleskva mannequin commented Oct 6, 2018

tirkarthi commented Mar 29, 2019

tirkarthi commented May 5, 2019

vadmium commented Mar 14, 2021

ambv commented Jun 7, 2022

ambv commented Jun 7, 2022

ambv commented Jun 7, 2022

tiran commented Jun 7, 2022

AA-Turner commented Jun 7, 2022

ambv commented Jun 7, 2022

tiran commented Jun 7, 2022

pSub commented Jun 9, 2022

ambv commented Jun 9, 2022

pablogsal commented Jun 9, 2022 • edited

ambv commented Jun 9, 2022

ambv commented Jun 9, 2022

pablogsal commented Jun 9, 2022 •

edited