Python: Port insecure default protocol #5246

yoff · 2021-02-23T18:45:07Z

The world has moved on, so this query is out of date in that its premise (that default constructors are insecure) has been invalidated (by the addition of fluent APIs to change defaults after construction). Therefor the QL help has been updated and the query has been limited to flag problems only when the version is older than the introduction of the fluent API.

Hence, this PR does the following:

use API graphs
update .qlhelp-file
limit to versions below 3.4
move tests to its own directory to only test on old version

- use API graphs - update .qlhelp-file - limit to versions below 3.4 - move tests to its own directory to only test on old version

felicitymay

Thanks for including a change note and for updating the query help file 😄

I've made a few text suggestions, but wasn't able to see the output of the Qhelp preview check run as this no longer seems to generate an artifact. I've flagged this to the CodeQL core team in case they have any suggestions.

python/change-notes/2021-02-23-port-insecure-default-protocol.md

felicitymay · 2021-02-24T09:17:24Z

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

            Note that <code>ssl.wrap_socket</code> has been deprecated in
            Python 3.7. A preferred alternative is to use
            <code>ssl.SSLContext</code>, which is supported in Python 2.7.9 and
-            3.2 and later versions.
+            3.2 and later versions or the convenience function
+            <code>ssl.create_default_context</code>, which is supported in Python
+            3.4 and later versions.
          </p>


Unfortunately, I can't add a suggestion here - GitHub complains about "deleted lines" which seems like a bug (in the tooltip if not in the functionality).

This is getting a little overlong and complex. I'd suggest breaking out into bullets for clarity and readability.

Note that <code>ssl.wrap_socket</code> has been deprecated in Python 3.7. The recommended alternatives are:</p> <ul> <li><code>ssl.SSLContext</code> - supported in Python 2.7.9, 3.2, and later versions</li> <li><code>ssl.create_default_context</code> - a convenience function, is supported in Python 3.4 and later versions.</li> </ul>

Thanks, I hope we will be able to see the result of my attempt at adapting this :-)

Co-authored-by: Felicity Chapman <felicitymay@github.com>

RasmusWL

I had a look at our discussion from last time you looked at this query (#3580). Our conclusion back then was that even though TLS 1.2 can be used in an insecure manner, we should only alert on version < TLS 1.2.

So TLS 1.1 should also be alerted on, and from my reading of https://docs.python.org/3.9/library/ssl.html#ssl.SSLContext, TLSv1.1 and TLSv1 are allowed with the default ssl.SSLContext() call.

However, it seems like newer versions of the API expects bit-fiddling with the options field -- is that maybe what you've referred to as fluent API? and maybe that was the change that happened in Python 3.4? In any circumstance, I would really like you to add a comment to the QL code explaining what happened in Python 3.4 😊

ctx = ssl.SSLContext()
ctx.options |= ssl.OP_NO_TLSv1
ctx.options |= ssl.OP_NO_TLSv1_1

Is the plan that we will handle such cases with the py/insecure-protocol query? and therefore, that this query should only alert on old code that doesn't support this bit-fiddling of the options field? (since clearly, just spotting the context creation with ssl.SSLContext() is not enough information to tell if that context is going to be secure or not)

I think it's all starting to come together for me, but I would have really liked for you to explain this stuff (in comment in the code, so we can see it in the future), instead of me doing detective work to figure it out 😄

It took me a long time to understand that the test-file InsecureProtocol was a direct copy of the old test-file. I added a few comments for cleaning up the test-file, since I thought this was something new you just wrote. I think this might be a great time to clean up the test-file anyway, but now you know why I have been so nit-picky.

RasmusWL · 2021-02-25T10:49:23Z

python/ql/test/query-tests/Security/CWE-327-py2/InsecureProtocol.py

+# not relevant
+wrap_socket(ssl_version=ssl.PROTOCOL_SSLv3)
+wrap_socket(ssl_version=ssl.PROTOCOL_TLSv1)
+wrap_socket(ssl_version=ssl.PROTOCOL_SSLv2)
+
+Context(SSL.SSLv3_METHOD)
+Context(SSL.TLSv1_METHOD)
+Context(SSL.SSLv2_METHOD)
+Context(SSL.SSLv23_METHOD)


nitpick:
What are these? If they're just your own verification that API graphs actually work, I don't think they should be in the query, but in some tests of API graphs 😉

python/ql/test/query-tests/Security/CWE-327-py2/options

RasmusWL · 2021-02-25T10:58:24Z

python/ql/test/query-tests/Security/CWE-327-py2/InsecureProtocol.py

+# FP for insecure default
+ssl.SSLContext(ssl.SSLv23_METHOD)


To my understanding, with Python 2.7.16 this is the same as using SSLContext(), which is the same as SSLContext(ssl.PROTOCOL_TLS). So I think those cases should by under the # possibly insecure default heading, and it should be explained they actually do the same thing.

Hmm, probably the heading should be something else... but grouping things together that does the same thing seems reasonable 😊

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

python/change-notes/2021-02-23-port-insecure-default-protocol.md

RasmusWL · 2021-02-25T11:11:56Z

python/ql/test/query-tests/Security/CWE-327-py2/InsecureProtocol.py

@@ -0,0 +1,52 @@
+import ssl
+from pyOpenSSL import SSL


I don't see any mention of pyOpenSSL in the insecure default protocol... does that means that default values of pyOpenSSL is always safe?

Yes, in that SSL.SSLContext requires a protocol argument; there is no default.

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff · 2021-02-26T08:56:27Z

I think it's all starting to come together for me, but I would have really liked for you to explain this stuff (in comment in the code, so we can see it in the future), instead of me doing detective work to figure it out 😄

I guess my hope that it would be explained by this comment was a bit naive. I will add a comment at the top of the query explaining that it only makes sense for older version of Python.

RasmusWL · 2021-02-26T09:17:08Z

I will add a comment at the top of the query explaining that it only makes sense for older version of Python.

👍 This, and the interplay with py/insecure-protocol should also be highlighted in the qhelp file. Possibly along with an example of how the new Python 3.4+ API allows secure usage of default values (I guess that's a matter of how the qhelp file writing turns out)

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

python/change-notes/2021-02-23-port-insecure-default-protocol.md

yoff · 2021-02-26T19:21:35Z

I explained the interplay in the .ql-file rather than in the help as it felt more natural. I did add an example of recommended use, though, that felt natural too.

yoff · 2021-03-01T15:39:57Z

@RasmusWL, thanks for digging into this, I have now revised the PR based on your insights. For the record, this is basically that for this query attention should be restricted to the call ssl.wrap_socket, which is the only way to create a socket with a default that cannot subsequently be modified.

RasmusWL

I think all the files this PR touches upon (especially the qhelp file, and the example) needs to have all references to SSLContext removed, maybe except for an example of how to use its' wrap_socket method in a safe manner. This query doesn't report on it anymore, so let's keep make our documentation match that as well 😉

Also just FYI, the example you added should have this change

-context.minimum_version(ssl.TLSVersion.TLSv1_2)
+context.minimum_version = ssl.TLSVersion.TLSv1_2

python/change-notes/2021-02-23-port-insecure-default-protocol.md

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff · 2021-03-03T17:38:18Z

@felicitymay Thank you for your comments, unfortunately much of the material has now been rewritten. I would be grateful for another look :-)

Ah, I just noted the new procedure for requesting this :-)

github-actions · 2021-03-03T17:43:03Z

Hello @github/docs-content-codeql : this PR is ready for docs review.

RasmusWL

This part of the qhelp file still needs to be changed so it doesn't talk about how default params of ssl.SSLContext are bad:

codeql/python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

Lines 27 to 66 in 3dd34c9

    
             <p> 
        
               The following code shows two different ways of setting up a connection 
        
               using SSL or TLS. They are both potentially insecure because the 
        
               default version is used. 
        
                </p> 
        
                <sample src="examples/insecure_default_protocol.py" /> 
        
                <p> 
        
                  Both of the cases above should be updated to use a secure protocol 
        
                  instead, for instance by specifying 
        
                  <code>ssl_version=PROTOCOL_TLSv1_2</code> as a keyword argument. 
        
                </p> 
        
                <p> 
        
                  The latter example can also be made secure by modifying the created 
        
                  context before it is used to create a connection and so will not be 
        
                  flagged by this query. However, if a connection is created before 
        
                  the context has been secured (e.g. by setting the value of <code>minimum_version</code>), 
        
                  then the code should be flagged by the query <code>py/insecure-protocol</code>. 
        
                </p> 
        
                <p> 
        
                  Note that <code>ssl.wrap_socket</code> has been deprecated in 
        
                  Python 3.7. The recommended alternatives are: 
        
                </p> 
        
                <ul> 
        
                  <li><code>ssl.SSLContext</code> - supported in Python 2.7.9, 
        
                        3.2, and later versions</li> 
        
                  <li><code>ssl.create_default_context</code> - a convenience function, 
        
                        supported in Python 3.4 and later versions.</li> 
        
                </ul> 
        
                <p> 
        
                  Note also that, even using these alternatives, it is recommended to 
        
                  ensure that a safe protocol is being used. The following code illustrates 
        
                  how to use either flags (available since Python 3.2) or the `minimum_version` 
        
                  field (favored since Python 3.7) to restrict the protocols accepted when 
        
                  creating a connection. 
        
                </p> 
        
                <sample src="examples/secure_default_protocol.py" /> 
        
           </example>

☝️ is basically what I tried to say in the non-example part of #5246 (review)

I'm not sure whether any of the references are no longer needed?

yoff · 2021-03-07T08:42:31Z

I think both examples are still valid and I like how this enabled a natural link to py/insecure-protocol after all.

felicitymay · 2021-03-08T11:13:48Z

@adityasharad - somehow this comment: #5246 (comment) wasn't picked up by our workflow. I'm not sure what the problem is but can see that the team name doesn't show up in bold as it would in a normal comment. (Update: Shati pointed out that this might be because the author of the comment is not a member of the github organization.)

felicitymay

A couple of small text suggestions, a question, and a possible bug in the qhelp -> markdown conversion. Otherwise, the text looks generally clear.

python/change-notes/2021-02-23-port-insecure-default-protocol.md

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

felicitymay · 2021-03-08T11:39:46Z

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

+            The latter example can also be made secure by modifying the created
+            context before it is used to create a connection and so will not be
+            flagged by this query. However, if a connection is created before


It's not clear to me from this text whether we mean the second example is never flagged by the query because it might be used in a secure way, or the second example is not flagged by the query if it is used in a secure way.

It is the first option. This query does not use data flow, so it now only flags cases that are immediately insecure. The other query referred to in the next sentence, py/insecure-protocol, uses data flow to detect if a context is made secure before it is used.

I have attempted a clarification now.

felicitymay · 2021-03-08T11:43:17Z

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

+            <li><code>ssl.create_default_context</code> - a convenience function,
+                  supported in Python 3.4 and later versions.</li>
+          </ul>
+          <p>


There is a bug in the markdown generated by the Qhelp test here. Note the missing blank line after the bullet list that means that this paragraph is run into the second bullet. I don't think this is an error in the qhelp file.

Note that `ssl.wrap_socket` has been deprecated in Python 3.7. The recommended alternatives are: * `ssl.SSLContext` - supported in Python 2.7.9, 3.2, and later versions * `ssl.create_default_context` - a convenience function, supported in Python 3.4 and later versions. Note also that, even using these alternatives, it is recommended to ensure that a safe protocol is being used. The following code illustrates how to use either flags (available since Python 3.2) or the \`minimum_version\` field (favored since Python 3.7) to restrict the protocols accepted when creating a connection.

I can try inserting a blank line in the help file and see what happens..

Thanks. That doesn't appear to have made any difference 😞

I'm not sure where we should report this bug in the CodeQL CLI, but it doesn't look as if it should block this PR.

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

adityasharad · 2021-03-08T16:09:47Z

@adityasharad - somehow this comment: #5246 (comment) wasn't picked up by our workflow. I'm not sure what the problem is but can see that the team name doesn't show up in bold as it would in a normal comment. (Update: Shati pointed out that this might be because the author of the comment is not a member of the github organization.)

@felicitymay it was the colon immediately after the team name. Adding a space seems to fix it, will update the workflow.

Co-authored-by: Felicity Chapman <felicitymay@github.com>

hopefully this will generate better markdown

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

felicitymay

Thanks for all the changes. I spotted one more small thing in the text but otherwise it looks okay to me 😄

Co-authored-by: Felicity Chapman <felicitymay@github.com>

tausbn

One minor comment on the query itself, otherwise LGTM.

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

…ff/codeql into python-port-insecure-default-protocol

RasmusWL · 2021-03-16T12:51:01Z

I think both examples are still valid and I like how this enabled a natural link to py/insecure-protocol after all.

Reading over the text and examples again, it does look ok 👍

RasmusWL · 2021-03-16T13:11:16Z

🤔 removing the request for review from Taus didn't allow me to merge this PR still 😂

Out of date

Python: Port insecure default protocol

b28544d

- use API graphs - update .qlhelp-file - limit to versions below 3.4 - move tests to its own directory to only test on old version

yoff requested review from felicitymay and a team as code owners February 23, 2021 18:45

github-actions bot added documentation Python labels Feb 23, 2021

Python: add change note

6abbb50

felicitymay reviewed Feb 24, 2021

View reviewed changes

yoff and others added 4 commits February 24, 2021 19:57

Update python/change-notes/2021-02-23-port-insecure-default-protocol.md

89d0724

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Python: Improve qlhelp according to review

1065716

Python: Rearrange directories

bf3e5fc

Python: Move <ul> outside of <p>

1929880

RasmusWL requested changes Feb 25, 2021

View reviewed changes

yoff and others added 2 commits February 26, 2021 08:53

Update python/ql/test/query-tests/Security/CWE-327-py2/options

a067adb

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Python: fix spelling

311149a

yoff and others added 2 commits February 26, 2021 10:56

Update python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

7f7320a

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update python/change-notes/2021-02-23-port-insecure-default-protocol.md

9a9bda1

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff commented Feb 26, 2021

View reviewed changes

python/change-notes/2021-02-23-port-insecure-default-protocol.md Outdated Show resolved Hide resolved

yoff and others added 3 commits February 26, 2021 18:39

Update python/change-notes/2021-02-23-port-insecure-default-protocol.md

1670fa0

Python: Clean up tests and add comment

9533c92

Python: Update help and add example

8b68912

yoff requested review from RasmusWL and felicitymay February 26, 2021 19:21

yoff mentioned this pull request Feb 26, 2021

Python: port py/insecure-protocol #5284

Merged

8 tasks

Python: restrict attention to ss.wrap_socket

38748f9

RasmusWL reviewed Mar 2, 2021

View reviewed changes

python/change-notes/2021-02-23-port-insecure-default-protocol.md Outdated Show resolved Hide resolved

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp Outdated Show resolved Hide resolved

Apply suggestions from code review

078fbcc

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 3, 2021

RasmusWL requested changes Mar 4, 2021

View reviewed changes

felicitymay reviewed Mar 8, 2021

View reviewed changes

yoff and others added 6 commits March 9, 2021 13:19

Update python/change-notes/2021-02-23-port-insecure-default-protocol.md

d5b304c

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Update python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

b6257ed

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Update python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

88784fb

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Update python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

fd5ac13

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Python: add linebreak to qhelp file

a16de26

hopefully this will generate better markdown

Python: Attempt to clarify help

8b25806

yoff requested a review from felicitymay March 9, 2021 12:30

felicitymay reviewed Mar 9, 2021

View reviewed changes

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp Outdated Show resolved Hide resolved

felicitymay previously approved these changes Mar 9, 2021

View reviewed changes

Update python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

dfdf034

Co-authored-by: Felicity Chapman <felicitymay@github.com>

yoff dismissed felicitymay’s stale review via dfdf034 March 9, 2021 15:44

tausbn previously requested changes Mar 10, 2021

View reviewed changes

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql Outdated Show resolved Hide resolved

yoff added 2 commits March 10, 2021 15:58

Python: simplify query code as suggested by review

e726ff4

Merge branch 'python-port-insecure-default-protocol' of github.com:yo…

fe975f2

…ff/codeql into python-port-insecure-default-protocol

yoff requested review from tausbn and RasmusWL March 10, 2021 15:02

RasmusWL approved these changes Mar 16, 2021

View reviewed changes

RasmusWL removed the request for review from tausbn March 16, 2021 13:10

RasmusWL merged commit 5097836 into github:main Mar 16, 2021

	<p>
	The following code shows two different ways of setting up a connection
	using SSL or TLS. They are both potentially insecure because the
	default version is used.
	</p>

	<sample src="examples/insecure_default_protocol.py" />

	<p>
	Both of the cases above should be updated to use a secure protocol
	instead, for instance by specifying
	<code>ssl_version=PROTOCOL_TLSv1_2</code> as a keyword argument.
	</p>
	<p>
	The latter example can also be made secure by modifying the created
	context before it is used to create a connection and so will not be
	flagged by this query. However, if a connection is created before
	the context has been secured (e.g. by setting the value of <code>minimum_version</code>),
	then the code should be flagged by the query <code>py/insecure-protocol</code>.
	</p>
	<p>
	Note that <code>ssl.wrap_socket</code> has been deprecated in
	Python 3.7. The recommended alternatives are:
	</p>
	<ul>
	<li><code>ssl.SSLContext</code> - supported in Python 2.7.9,
	3.2, and later versions</li>
	<li><code>ssl.create_default_context</code> - a convenience function,
	supported in Python 3.4 and later versions.</li>
	</ul>
	<p>
	Note also that, even using these alternatives, it is recommended to
	ensure that a safe protocol is being used. The following code illustrates
	how to use either flags (available since Python 3.2) or the `minimum_version`
	field (favored since Python 3.7) to restrict the protocols accepted when
	creating a connection.
	</p>

	<sample src="examples/secure_default_protocol.py" />
	</example>

Python: Port insecure default protocol #5246

Python: Port insecure default protocol #5246

Uh oh!

Conversation

yoff commented Feb 23, 2021

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoff commented Feb 26, 2021

Uh oh!

RasmusWL commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yoff commented Feb 26, 2021

Uh oh!

yoff commented Mar 1, 2021

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 3, 2021

Uh oh!

github-actions bot commented Mar 3, 2021 • edited by adityasharad Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RasmusWL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoff commented Mar 7, 2021

Uh oh!

felicitymay commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adityasharad commented Mar 8, 2021

RasmusWL left a comment •

edited

Loading

RasmusWL commented Feb 26, 2021 •

edited

Loading

github-actions bot commented Mar 3, 2021 •

edited by adityasharad

Loading

RasmusWL left a comment •

edited

Loading

felicitymay commented Mar 8, 2021 •

edited

Loading