4 ms·

I just wrote this up elsewhere, but wow, is that Python code awful. It uses string interpolation to build a curl command line. Then it passes that string to a s

I just wrote this up elsewhere, but wow, is that Python code awful. It uses string interpolation to build a curl command line. Then it passes that string to a subprocess.Popen(cmd,shell=True). For those who don’t speak Python, that’s saying “run the string passed in here as a shell command exactly as if someone typed it into a bash prompt”. The right thing to to there would have been to create a list of arguments and pass that list in instead of a string, and set shell=False so that the list gets sent straight to the underlying syscall without being parsed and interpreted as a command line, with all the edge cases that brings. As worryingly, the free, open source Bandit Python static analyzer immediately flagged this code when I tested it locally. If Palo Alto had any kind of static analysis in their build pipeline, this would have failed. If they’re not doing that, what else are they not doing?

yrro 3y ago
Couldnt have put it better myself. The sheer incompetence is staggering and frankly it's depressing that these giant corporations are so good at selling their steaming piles of shit to companies who should really know better. If PAN-OS was open source then no one would want to be within 10,000 miles of it!
- kstrauser 3y ago
  If it were open source, I could imagine sending some PRs there way, wishing them well, then running away to a safe distance. That this is the output of an $89B security company is mind boggling.
FreakLegion 3y ago
When I left PANW in the late 2010s the platform team was still on Python 2.6, I think, and the snippets in this write-up are valid 2.x code. It's very possible that this hasn't been touched in a decade beyond whatever was necessary to get to 3.6, which is itself 2+ years past its shelf date. That's a bigger and slightly more abstract problem than not running a mediocre linter.
- kstrauser 3y ago
  Oof. That sounds in dire need of some dev resources. I disagree with that last bit though. First, I think bandit's more than a "mediocre linter". More importantly, it's a free, open source, CI-friendly app that could be plugged into any reasonably normal build pipeline. It would have caught that. If bandit would have, I'm sure any number of other more advanced tools would too. The equally free ruff checker reports: "S604 Function call with `shell=True` parameter identified, security issue". What I take from that is that there's no reasonable amount of code analysis at all. If they don't even run linters, I'm sure something like mypy is out of the question. Are there unit tests? I don't have confidence of it based on this code alone.
  - FreakLegion 3y ago
    I've tried Bandit on a few codebases and each time it's yielded thousands of false positives and no actual findings. In the best cases it's something like hashlib.sha1, where you should set usedforsecurity=False anyway. That's at least an actionable, if ineffectual, alert. You can make the code clearer, and the warning goes away. But in the worst cases it's something like having a variable named "token" (You idiot! Is that a password in your source?), or constructing SQL queries, which by design may never accept user input but, you know, Bandit can't determine that statically, so it goes ahead and throws an absolute fit anyway. Conversely, I apply Pylint and mypy aggressively, because what they find almost always should be changed. But Bandit's findings are overwhelmingly wrong in context, so all you can do is litter your code with ignore directives. For reference, in my current codebase of 175k lines, there are 52 Pylint and 66 mypy ignores (with mypy at max strictness). Bandit would need 1,823. Palo Alto absolutely should do better, but Bandit, or probably any "security" linter, isn't the answer.
    - kstrauser 3y ago
      I've had a vastly different experience. Filtering only high level and high confidence findings can get rid of lots of chaff. But regardless, even ruff looks for that specific setting. And even if there were only 2 real findings out of 1,823, if it's in a product that you're selling to enterprise security customers, there's not an excuse for not having had someone look at the results of bandit or something like it and ruling out the false positives. They have $89B worth of reputation to lose with things like this.

yrro 3y ago

Couldnt have put it better myself. The sheer incompetence is staggering and frankly it's depressing that these giant corporations are so good at selling their steaming piles of shit to companies who should really know better. If PAN-OS was open source then no one would want to be within 10,000 miles of it!

kstrauser 3y ago
If it were open source, I could imagine sending some PRs there way, wishing them well, then running away to a safe distance. That this is the output of an $89B security company is mind boggling.

FreakLegion 3y ago

When I left PANW in the late 2010s the platform team was still on Python 2.6, I think, and the snippets in this write-up are valid 2.x code. It's very possible that this hasn't been touched in a decade beyond whatever was necessary to get to 3.6, which is itself 2+ years past its shelf date. That's a bigger and slightly more abstract problem than not running a mediocre linter.

kstrauser 3y ago
Oof. That sounds in dire need of some dev resources. I disagree with that last bit though. First, I think bandit's more than a "mediocre linter". More importantly, it's a free, open source, CI-friendly app that could be plugged into any reasonably normal build pipeline. It would have caught that. If bandit would have, I'm sure any number of other more advanced tools would too. The equally free ruff checker reports: "S604 Function call with `shell=True` parameter identified, security issue". What I take from that is that there's no reasonable amount of code analysis at all. If they don't even run linters, I'm sure something like mypy is out of the question. Are there unit tests? I don't have confidence of it based on this code alone.
- FreakLegion 3y ago
  I've tried Bandit on a few codebases and each time it's yielded thousands of false positives and no actual findings. In the best cases it's something like hashlib.sha1, where you should set usedforsecurity=False anyway. That's at least an actionable, if ineffectual, alert. You can make the code clearer, and the warning goes away. But in the worst cases it's something like having a variable named "token" (You idiot! Is that a password in your source?), or constructing SQL queries, which by design may never accept user input but, you know, Bandit can't determine that statically, so it goes ahead and throws an absolute fit anyway. Conversely, I apply Pylint and mypy aggressively, because what they find almost always should be changed. But Bandit's findings are overwhelmingly wrong in context, so all you can do is litter your code with ignore directives. For reference, in my current codebase of 175k lines, there are 52 Pylint and 66 mypy ignores (with mypy at max strictness). Bandit would need 1,823. Palo Alto absolutely should do better, but Bandit, or probably any "security" linter, isn't the answer.
  - kstrauser 3y ago
    I've had a vastly different experience. Filtering only high level and high confidence findings can get rid of lots of chaff. But regardless, even ruff looks for that specific setting. And even if there were only 2 real findings out of 1,823, if it's in a product that you're selling to enterprise security customers, there's not an excuse for not having had someone look at the results of bandit or something like it and ruling out the false positives. They have $89B worth of reputation to lose with things like this.