gh-97612: Fix shell injection in get-remote-certificate.py #97613

vstinner · 2022-09-28T10:15:39Z

Fix a shell code injection vulnerability in the
get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt.

Remove the Windows code path to send "quit" on stdin to the "openssl
s_client" command: use DEVNULL on all platforms instead.

Issue: [security] Tools/scripts/get-remote-certificate.py is vulnerable to shell code injection #97612

vstinner · 2022-09-28T10:16:34Z

@gpshead: Would you mind to review this fix?

The fix should also be backported to the 3.7 branch, but there is a conflict, so I will do the backport manually once this PR is merged.

cc @calebshortt

zware · 2022-09-28T14:21:04Z

What's the point of even keeping this script? Has anyone used it in the past 12 years?

vstinner · 2022-09-28T15:20:15Z

What's the point of even keeping this script? Has anyone used it in the past 12 years?

In Python 3.12, IMO it's fine to consider removing the script. But for stable Python versions (3.7-3.11), IMO we should fix the script.

I'm working on proposal to remove most Tools/scripts/ scripts, but first I have to look into these scripts to decide what to do with them, categorize them, etc. There are more than 70 scripts.

gpshead · 2022-09-28T16:05:56Z

Tools/scripts/get-remote-certificate.py

-            'openssl s_client -connect "%s:%s" -showcerts < /dev/null' %
-            (host, port))
+    cmd = ['openssl', 's_client', '-connect', '%s:%s' % (host, port), '-showcerts']
+    status, output = subproc(cmd)


on windows stdin was hooked up to a file containing "quit\n". was that necessary? If so, use your added subproc stdin= support from above to do that. otherwise, get rid of the added subprob stdin= support to make this PR smaller.

Right, I removed the now unused stdin parameter.

This script is used to connect to a HTTPS server. Saying "quit\n" to a HTTP server makes no sense to me. Maybe on Windows, previously the command was blocked until the user wrotes something and then press ENTER? But with this change, the stdin is replaced with DEVNULL (NUL device on Windows) and so it should no longer block.

First, I basically rewrote the whole script. I don't get half of the code. For example, it runs the "x509" to get the same output than the input... it doesn't make any sense to me. But I tried to keep this change as small as possible...

Then I think that we should just remove the script in the main branch :-)

My main motivation for this change is to prevent users to report the same issue later, even if the impact of the vulnerability is basically non existent since I don't imagine that anyone runs such script on a production server with inputs taken from the Internet...

In that case, I'd take the current report as reason to remove the script from main, and the next report as reason to remove it from other branches. Or just go ahead and remove it everywhere. It's not worth fixing.

Since the fix is straightforward, I prefer first to fix the script in all branches.

I created https://discuss.python.org/t/remove-outdated-tools-scripts-scripts/19571 discussion to propose removing outdated example scripts.

Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead.

vstinner · 2022-09-28T20:07:26Z

@gpshead: Would you mind to review the updated PR?

miss-islington · 2022-09-28T23:17:30Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9, 3.10, 3.11.
🐍🍒⛏🤖

bedevere-bot · 2022-09-28T23:17:38Z

GH-97630 is a backport of this pull request to the 3.11 branch.

…honGH-97613) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-bot · 2022-09-28T23:17:44Z

GH-97631 is a backport of this pull request to the 3.10 branch.

…honGH-97613) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-bot · 2022-09-28T23:17:50Z

GH-97632 is a backport of this pull request to the 3.9 branch.

…honGH-97613) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-bot · 2022-09-28T23:17:55Z

GH-97633 is a backport of this pull request to the 3.8 branch.

…honGH-97613) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

miss-islington · 2022-09-28T23:19:06Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2022-09-28T23:19:09Z

Sorry, @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 83a0f44ffd8b398673ae56c310cf5768d359c341 3.7

bedevere-bot · 2022-09-28T23:22:17Z

GH-97634 is a backport of this pull request to the 3.7 branch.

vstinner · 2022-09-28T23:22:50Z

@zware: I prefer the conservative approach, first fix the vulnerability, then discuss removing outdated scripts.

Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

…97613) (GH-97632) gh-97612: Fix shell injection in get-remote-certificate.py (GH-97613) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

…97613) (GH-97633) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44) Co-authored-by: Victor Stinner <vstinner@python.org>

) (#97634) Fix a shell code injection vulnerability in the get-remote-certificate.py example script. The script no longer uses a shell to run "openssl" commands. Issue reported and initial fix by Caleb Shortt. Remove the Windows code path to send "quit" on stdin to the "openssl s_client" command: use DEVNULL on all platforms instead. Co-authored-by: Caleb Shortt <caleb@rgauge.com> (cherry picked from commit 83a0f44)

vstinner added type-security A security issue needs backport to 3.8 needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 28, 2022

bedevere-bot added the awaiting core review label Sep 28, 2022

gpshead reviewed Sep 28, 2022

View reviewed changes

gpshead approved these changes Sep 28, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 28, 2022

vstinner merged commit 83a0f44 into python:main Sep 28, 2022

bedevere-bot removed the awaiting merge label Sep 28, 2022

vstinner deleted the get_certs branch September 28, 2022 23:17

bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 28, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 28, 2022

bedevere-bot removed the needs backport to 3.9 only security fixes label Sep 28, 2022

bedevere-bot removed the needs backport to 3.8 label Sep 28, 2022

vstinner added the needs backport to 3.7 label Sep 28, 2022

miss-islington assigned vstinner Sep 28, 2022

bedevere-bot removed the needs backport to 3.7 label Sep 28, 2022

kumaraditya303 mentioned this pull request Oct 1, 2022

Fix for CVE-2022-37460 - Removed "shell=True", made args a list, and revised to handle stdin in function #96014

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-97612: Fix shell injection in get-remote-certificate.py #97613

gh-97612: Fix shell injection in get-remote-certificate.py #97613

vstinner commented Sep 28, 2022 •

edited by bedevere-bot

Loading

vstinner commented Sep 28, 2022

zware commented Sep 28, 2022

vstinner commented Sep 28, 2022

gpshead Sep 28, 2022

vstinner Sep 28, 2022

zware Sep 28, 2022

vstinner Sep 28, 2022

vstinner Sep 29, 2022

vstinner commented Sep 28, 2022

miss-islington commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

miss-islington commented Sep 28, 2022

miss-islington commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

vstinner commented Sep 28, 2022

gh-97612: Fix shell injection in get-remote-certificate.py #97613

gh-97612: Fix shell injection in get-remote-certificate.py #97613

Conversation

vstinner commented Sep 28, 2022 • edited by bedevere-bot Loading

vstinner commented Sep 28, 2022

zware commented Sep 28, 2022

vstinner commented Sep 28, 2022

gpshead Sep 28, 2022

Choose a reason for hiding this comment

vstinner Sep 28, 2022

Choose a reason for hiding this comment

zware Sep 28, 2022

Choose a reason for hiding this comment

vstinner Sep 28, 2022

Choose a reason for hiding this comment

vstinner Sep 29, 2022

Choose a reason for hiding this comment

vstinner commented Sep 28, 2022

miss-islington commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

miss-islington commented Sep 28, 2022

miss-islington commented Sep 28, 2022

bedevere-bot commented Sep 28, 2022

vstinner commented Sep 28, 2022

vstinner commented Sep 28, 2022 •

edited by bedevere-bot

Loading