Xqt has submitted this change. (
https://gerrit.wikimedia.org/r/c/pywikibot/core/+/1210626?usp=email )
Change subject: SEC: parse password lines as literals & update default
PASS_BASENAME
......................................................................
SEC: parse password lines as literals & update default PASS_BASENAME
- Replaced eval() with ast.literal_eval() to prevent remote code execution.
- Added sanity checks for line length and comma count.
- BotPassword objects are safely reconstructed after parsing.
- Raises ValueError for malformed password line instead passing silently
- Default PASS_BASENAME changed from 'user-password.py' to 'user-password.cfg'.
- Verified with unit test ensuring no code execution occurs from password file.
- Updated docstrings and versionchanged notices.
Patch backported from master
Bug: T410753
Change-Id: I685320ac3ea837023fc41278c48916786d546eff
---
M pywikibot/login.py
M pywikibot/scripts/generate_user_files.py
M tests/generate_user_files_tests.py
M tests/login_tests.py
4 files changed, 74 insertions(+), 11 deletions(-)
Approvals:
jenkins-bot: Verified
Xqt: Looks good to me, approved
diff --git a/pywikibot/login.py b/pywikibot/login.py
index 94650e9..e7eb88c 100644
--- a/pywikibot/login.py
+++ b/pywikibot/login.py
@@ -10,6 +10,7 @@
import os
import re
import webbrowser
+from ast import literal_eval
from enum import IntEnum
from pathlib import Path
from textwrap import fill
@@ -33,6 +34,8 @@
except ImportError as e:
mwoauth = e
+TEST_RUNNING = os.environ.get('PYWIKIBOT_TEST_RUNNING', '0') == '1'
+
class _PasswordFileWarning(UserWarning):
@@ -202,7 +205,7 @@
will be used, so default usernames should occur above specific
usernames.
- .. note:: For BotPasswords the password should be given as a
+ .. note:: For BotPasswords the password should be given like a
:class:`BotPassword` object.
The file must be either encoded in ASCII or UTF-8.
@@ -215,10 +218,17 @@
('my_username', BotPassword('my_suffix', 'my_password'))
.. versionchanged:: 10.2
- raises ValueError instead of AttributeError if password_file
- is not set
+ Raises ValueError instead of AttributeError if password_file
+ is not set.
+ .. versionchanged:: 10.7.1
+ Due to vulnerability issue the password lines are no longer
+ evaluated as Python source but parsed as literals.
+ Raises ValueError if an exception occurs while evaluating a
+ password line.
- :raises ValueError: `password_file` is not set in the user-config.py
+
+ :raises ValueError: `password_file` is not set in the
+ ``user-config.py`` or entries are critical malformed
:raises FileNotFoundError: password file does not exist
"""
if config.password_file is None:
@@ -247,15 +257,24 @@
lines = password_path.read_text('utf-8').splitlines()
line_len = len(lines)
-
for n, line in enumerate(reversed(lines)):
- if not line.strip() or line.startswith('#'):
+ line = line.strip()
+ if not line or line.startswith('#'):
continue
+ # sanity check
+ if (line.count(',') > 3 or len(line) > 250) and not TEST_RUNNING:
+ raise ValueError('Password line too long or too complex')
+
+ botpassword = 'BotPassword' in line
+ if botpassword:
+ line = line.replace('BotPassword', '')
+
try:
- entry = eval(line)
- except SyntaxError:
- entry = None
+ entry = literal_eval(line)
+ except Exception:
+ # catch the exceptions to not leak passwords to console or logs
+ raise ValueError('Invalid password line format') from None
if not isinstance(entry, tuple):
warn(f'Invalid tuple in line {line_len - n}',
@@ -268,6 +287,9 @@
_PasswordFileWarning, stacklevel=2)
continue
+ if botpassword:
+ entry = (entry[0], BotPassword(*entry[1]))
+
code, family, username, password = (
self.site.code, self.site.family.name)[:4 - entry_len] + entry
diff --git a/pywikibot/scripts/generate_user_files.py
b/pywikibot/scripts/generate_user_files.py
index 1395768..e5663d96 100755
--- a/pywikibot/scripts/generate_user_files.py
+++ b/pywikibot/scripts/generate_user_files.py
@@ -6,6 +6,9 @@
.. versionchanged:: 8.0
let user the choice which section to be copied.
Also EXTERNAL EDITOR SETTINGS section can be copied.
+.. versionchanged:: 10.7.1
+ Default ``PASS_BASENAME`` was changed from ``user-password.py`` to
+ ``user-password.cfg``
"""
#
# (C) Pywikibot team, 2010-2025
@@ -55,7 +58,7 @@
console_encoding = 'iso-8859-1'
USER_BASENAME = 'user-config.py'
-PASS_BASENAME = 'user-password.py'
+PASS_BASENAME = 'user-password.cfg'
def change_base_dir():
diff --git a/tests/generate_user_files_tests.py
b/tests/generate_user_files_tests.py
index 28b2575..609c108 100755
--- a/tests/generate_user_files_tests.py
+++ b/tests/generate_user_files_tests.py
@@ -30,7 +30,7 @@
def test_base_names(self) -> None:
"""Test basename constants."""
self.assertEndsWith(guf.USER_BASENAME, '.py')
- self.assertEndsWith(guf.PASS_BASENAME, '.py')
+ self.assertEndsWith(guf.PASS_BASENAME, '.cfg')
def test_config_test(self) -> None:
"""Test config text strings."""
diff --git a/tests/login_tests.py b/tests/login_tests.py
index 83ce47e..fa0ee65 100755
--- a/tests/login_tests.py
+++ b/tests/login_tests.py
@@ -10,6 +10,8 @@
#
from __future__ import annotations
+import builtins
+import uuid
from collections import defaultdict
from io import StringIO
from pathlib import Path
@@ -187,6 +189,42 @@
""", '~FakePassword')
self.assertEqual(obj.login_name, '~FakeUsername@~FakeSuffix')
+ def test_eval_security(self) -> None:
+ """Test security that password file does not use eval() function."""
+ # Test file will will be created for Python 3.10-3.13
+ # due to self.stat patch in setUp().
+ no_file = (3, 9) < PYTHON_VERSION < (3, 14)
+
+ builtins.exploit_value = False
+ exploit_code = (
+ "__import__('builtins').__dict__"
+ ".__setitem__('exploit_value', True)"
+ )
+ if not no_file:
+ exploit_filename = f'pwb_rce_{uuid.uuid4().hex[:8]}.txt'
+ exploit_file = Path(exploit_filename)
+ exploit_code = (
+ f"__import__('pathlib').Path('{exploit_filename}').touch() or "
+ + exploit_code
+ )
+
+ with self.subTest(test='Test ValueError'), \
+ self.assertRaisesRegex(ValueError,
+ 'Invalid password line format'):
+ self._test_pwfile(f"""
+ ('en', 'wikipedia', 'victim', {exploit_code})
+ """, None)
+
+ with self.subTest(test='Test value was modified'):
+ self.assertFalse(exploit_value) # noqa: F821
+
+ if not no_file:
+ with self.subTest(test='Test file exists'):
+ self.assertFalse(exploit_file.exists())
+
+ # cleanup file (should never happen)
+ exploit_file.unlink(missing_ok=True)
+
if __name__ == '__main__':
unittest.main()
--
To view, visit
https://gerrit.wikimedia.org/r/c/pywikibot/core/+/1210626?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.wikimedia.org/r/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: pywikibot/core
Gerrit-Branch: stable
Gerrit-Change-Id: I685320ac3ea837023fc41278c48916786d546eff
Gerrit-Change-Number: 1210626
Gerrit-PatchSet: 1
Gerrit-Owner: Xqt <[email protected]>
Gerrit-Reviewer: Xqt <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
Pywikibot-commits mailing list -- [email protected]
To unsubscribe send an email to [email protected]
