[flake8_bandit] Fix S412 false negatives for any imports from wsgiref.handlers and twisted.web.twcgi#19247
[flake8_bandit] Fix S412 false negatives for any imports from wsgiref.handlers and twisted.web.twcgi#19247danparizher wants to merge 2 commits intoastral-sh:mainfrom
flake8_bandit] Fix S412 false negatives for any imports from wsgiref.handlers and twisted.web.twcgi#19247Conversation
|
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this looks like what I had in mind in #19242. My only hesitations are that this is a bit of a divergence from the upstream linter and also an expansion of the rule. However, I think we might diverge in the general implementation of these rules, and we could gate the expansion behind preview if we're worried about that part.
I also think these are probably pretty niche modules to import, so I'm not sure how much impact this will have either way.
@MichaReiser what do you think? To summarize the issue:
- S412 only flags
from wsgiref.handlers import CGIHandler, not other ways of importing - The upstream linter also flags calls to the insecure code (
wsgiref.handlers.CGIHandler())
I suggested flagging the whole module import (this PR) as an easy fix that looks to be in line with our other S rules, but a more robust fix might be flagging calls like upstream. That could be a bigger change that might also affect our other S rules.
The rule itself is still preview Hmm not sure. Does the same security concern exist for
We should rename the rule if we do so because it then flags the usages and not the imports |
|
What claude thinks: DetailsI'll fetch the specific wsgiref handlers documentation to analyze each handler individually.Based on the wsgiref.handlers documentation and my HTTPoxy research, here's a specific analysis of each handler's vulnerability status:VULNERABLE Handlers:wsgiref.handlers.CGIHandlerVULNERABLE - This is explicitly mentioned in the HTTPoxy documentation as vulnerable. Python code must be deployed under CGI to be vulnerable, and this usually means using wsgiref.handlers.CGIHandler. This handler uses wsgiref.handlers.IISCGIHandlerVULNERABLE - This is a specialized subclass of wsgiref.handlers.BaseCGIHandlerVULNERABLE - This is the base class for CGI handlers that NOT VULNERABLE Handlers:wsgiref.handlers.SimpleHandlerNOT VULNERABLE - The documentation specifically states this is "designed for use with HTTP origin servers" and "If you are writing an HTTP server implementation, you will probably want to subclass this instead of BaseCGIHandler." Since regular WSGI environments keep user-supplied values in a separate WSGI 'environ' map, they are not vulnerable. The os.environ['HTTP_PROXY'] remains unchanged when a Proxy header is sent. wsgiref.handlers.BaseHandlerNOT VULNERABLE - This is an abstract base class for running WSGI applications. It's designed for HTTP origin servers, not CGI environments. The documentation shows it maintains separate environment handling and doesn't directly expose Key Distinction:The vulnerability comes down to whether the handler operates in a CGI environment where:
CGI-based handlers (CGIHandler, IISCGIHandler, BaseCGIHandler) are vulnerable because they operate in environments where this header-to-environment variable mapping occurs. HTTP origin server handlers (SimpleHandler, BaseHandler) are not vulnerable because they maintain proper separation between HTTP headers and environment variables, keeping user-supplied data in separate WSGI environ dictionaries. So I think outright banning the package isn't the right fix. |
🤦 My mistake, didn't notice that This is the implementation of class BaseCGIHandler(SimpleHandler):
origin_server = Falsebut I guess the I think we may need to rename and reimplement the rule to catch calls in that case. I suspect, but still haven't confirmed, that this is the case for the other |
Summary
Fixes #19242