Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates #250

ad-m · 2017-05-25T17:42:47Z

No description provided.

PlasmaPower · 2017-05-25T18:06:05Z

startup.d/10-git-pull.sh

+# Prepare code auditing environment
+REPO_URL=$(git remote get-url origin)
+TEST_DIR=$(mktemp -d)
+git clone "$REPO_URL" "$TEST_DIR";


Instead of a tmpdir, maybe just use a constant folder so we can just git pull instead of a fresh clone every time.

This may be embarrassing for future tests, which may leave data - junk.

https://stackoverflow.com/a/1210012/345059 could be used to speed up the temp clone as the repo grows

PlasmaPower · 2017-05-25T18:06:48Z

Note that this doesn't prevent malicious code changes, but rather just syntax errors. Still 👍.

reddraggone9

I'd really rather test each PR individually and just not merge them in the first place if they don't pass, but this is still better than what we have now.

reddraggone9 · 2017-05-25T17:56:55Z

startup.d/10-git-pull.sh

+pushd "$TEST_DIR";
+
+# Code auditing section
+python -m py_compile $(find . -name '*.py');


I don't think this will use the right python since (from what I see) we're using a virtualenv without actually modifying the environment (e.g. PATH).

Also, something like flake8 would pick up more problems (e.g. use of undefined variables).

Syntax analysis only needs a consistent version of Python. It deliberately does not implement PEP8 analysis, as it may be too rigorous and embarrassing.

I mentioned flake8 because it's what I use. I can see why you'd not want to fail on style though. Maybe use pyflakes directly instead? Regardless, like I said above, this is still better than what we have. We can refine it later.

You really do need to use the correct version of python though.

@ad-m @reddraggone9 yep the chaos venv python lives in /root/.virtualenvs/chaos/bin/python. We should do something so we don't have to hardcode it for too much longer in our script files

@amoffat , drop virtualenv at all 👍

reddraggone9 · 2017-05-25T18:01:11Z

startup.d/10-git-pull.sh

+
+# Prepare code auditing environment
+REPO_URL=$(git remote get-url origin)
+TEST_DIR=$(mktemp -d)


This should either be cleaned up so we're not wasting disk space or (preferably) use a single, fixed directory. With the latter option, you could fetch and hard reset to origin/master instead of cloning the entire repo every time.

You're right, it could be cleaned up.

reddraggone9 · 2017-05-25T18:08:05Z

startup.d/10-git-pull.sh

+
+# Code auditing section
+python -m py_compile $(find . -name '*.py');
+if [ "$?" != "0" ]


Just if [ $? ] will do here.

Nevermind me, I'm an idiot.

ad-m · 2017-05-25T18:24:02Z

@PlasmaPower , every syntax error interferes with our program, so I think it is malicious. Additional analysis will be available in the future.

PlasmaPower · 2017-05-25T18:26:22Z

@ad-m Malicious implies meaning evil, which most syntax errors aren't.

reddraggone9 · 2017-05-25T18:39:33Z

startup.d/10-git-pull.sh

@@ -16,6 +16,7 @@ fi

 # End code auditing section
 popd
+rm -r "$TEST_DIR";


But what if it exits up above? Also needs -f since git objects are write-protected.

It will removed too. See a175ecb#diff-8d12b14fde22f0fe54cea293ca1caa32R14 .

patricknelson · 2017-05-25T18:49:17Z

malicious:

characterized by malice; intending or intended to do harm.

Aside from the fact that this is still a useful PR, regardless of being incorrectly titled.

amoffat · 2017-05-25T18:56:33Z

@ad-m are you able to test this in the docker dev env to confirm that everything works as designed?

ad-m · 2017-05-25T19:02:17Z

@amoffat , I have never use docker yet.

ad-m · 2017-05-25T19:03:43Z

@patricknelson , fixed!

reddraggone9 · 2017-05-25T19:41:52Z

This will fail on files with spaces in their names:

$ mkdir test
$ cd test
$ touch test.py 'with space.py'
$ python3 -m py_compile $(find . -name '*.py');
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/runpy.py", line 170, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/local/lib/python3.5/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/usr/local/lib/python3.5/py_compile.py", line 186, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.5/py_compile.py", line 178, in main
    compile(filename, doraise=True)
  File "/usr/local/lib/python3.5/py_compile.py", line 122, in compile
    source_bytes = loader.get_data(file)
  File "<frozen importlib._bootstrap_external>", line 815, in get_data
FileNotFoundError: [Errno 2] No such file or directory: './with'

Working around this in the shell seems painful. It'd probably be a good idea to eventually rewrite these scripts in python.

ad-m · 2017-05-25T19:50:44Z

@reddraggone9 , fixed.

$ touch test.py 'with space.py'
$ find . -name '*.py' -print0 | xargs -0 python -m py_compile;
Sorry: IndentationError: unindent does not match any outer indentation level (chaos.py, line 61)
$ echo $?
123
$ rm chaos.py
$ find . -name '*.py' -print0 | xargs -0 python -m py_compile;
$ echo $?
0

patricknelson · 2017-05-25T19:54:25Z

Unvoted and revoted... will the vote count now? Relevant feature request: #196

reddraggone9 · 2017-05-25T19:54:49Z

You don't need to recast your vote anymore. See #117.

patricknelson · 2017-05-25T19:58:30Z

@reddraggone9 does this currently document the entirety of how voting works? https://github.com/chaosbot/chaos#voting

i.e. This means you can continuously push to a PR and it'll be valid, but the voting window closes entirely after 3hrs of opening the PR? Assuming this is is relative to that point in time:

Voting goes on for the duration of the voting window - currently 2 or 3 hours, depending on the local server time.

reddraggone9 · 2017-05-25T20:00:51Z

There are corner cases, but that's mostly right. The window gets reset every time you push to your repo, but old votes still count.

reddraggone9 · 2017-05-25T20:02:34Z

startup.d/10-git-pull.sh

+pushd "$TEST_DIR";
+
+# Code auditing section
+find . -name '*.py' -print0 | xargs -0 python -m py_compile;


Looks like I commented on the wrong PR.

python3 -m py_compile **/*.py would be cleaner.

@EtherTyper

#250: Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates Description: :ok_woman: PR passed with a vote of 15 for and 1 against, with a weighted total of 14.0 and a threshold of 6.2. Vote record: @EtherTyper: 1 @JarJak: 1 @Mursaat: 1 @PlasmaPower: 1 @ad-m: 1 @chickenchuck040: 1 @dcstone09: 1 @ike709: 1 @kochmaxence: -1 @patricknelson: 1 @reddraggone9: 1 @rhengles: 1 @rudehn: 1 @sorcio: 1 @viktorsec: 1 @xyproto: 1

chaosbot · 2017-05-25T22:31:41Z

🙆‍♀️ PR passed with a vote of 15 for and 1 against, with a weighted total of 14.0 and a threshold of 6.2.

See merge-commit 56ade60 for more details.

rudehn · 2017-05-26T11:36:04Z

-v, --verbose         be verbose; must be placed before a subcommand

fatal: The empty string is not a valid path
startup.d/10-git-pull.sh: 7: startup.d/10-git-pull.sh: pushd: not found
startup.d/10-git-pull.sh: 19: startup.d/10-git-pull.sh: popd: not found
Already on 'master'
From https://github.com/chaosbot/Chaos
ef69d70..c80f23f master -> origin/master
ERROR: provided hosts list is empty
ERROR: provided hosts list is empty

ad-m · 2017-05-26T11:40:36Z

@rudehn , where did you find it?

ad-m · 2017-05-26T11:49:36Z

git remote get-url was added on git/2.7.0. We have to rewrite that.

rudehn · 2017-05-26T12:38:11Z

@ad-m I found it in the logs. http://chaosthebot.com:8081

ad-m added 3 commits May 25, 2017 18:36

Add protection to syntax errors

ccbcbc2

Move order of syntax check

46251bb

Prevent code updates if malicious changes

093c71f

PlasmaPower reviewed May 25, 2017

View reviewed changes

reddraggone9 suggested changes May 25, 2017

View reviewed changes

reddraggone9 reviewed May 25, 2017

View reviewed changes

ad-m added 2 commits May 25, 2017 20:40

Clean up after auditing

a175ecb

Force remove audited code

cc74c5d

ad-m changed the title ~~Prevent malicious code updates~~ Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates May 25, 2017

Fix auditing with spaces in filename

17b0ad8

reddraggone9 reviewed May 25, 2017

View reviewed changes

reddraggone9 mentioned this pull request May 25, 2017

Fix PEP8 errors #248

Merged

chaosbot merged commit 56ade60 into Chaosthebot:master May 25, 2017

chaosbot added the accepted label May 25, 2017

reddraggone9 mentioned this pull request May 25, 2017

Add protection to syntax errors #247

Closed

ad-m deleted the prevent-code-changes branch May 26, 2017 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates #250

Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates #250

ad-m commented May 25, 2017

PlasmaPower May 25, 2017

ad-m May 25, 2017

amoffat May 25, 2017

PlasmaPower commented May 25, 2017

reddraggone9 left a comment

reddraggone9 May 25, 2017

ad-m May 25, 2017

reddraggone9 May 25, 2017

amoffat May 25, 2017

ad-m May 25, 2017

reddraggone9 May 25, 2017

ad-m May 25, 2017

reddraggone9 May 25, 2017

reddraggone9 May 25, 2017

ad-m commented May 25, 2017

PlasmaPower commented May 25, 2017

reddraggone9 May 25, 2017 •

edited

Loading

ad-m May 25, 2017

patricknelson commented May 25, 2017

amoffat commented May 25, 2017

ad-m commented May 25, 2017

ad-m commented May 25, 2017

reddraggone9 commented May 25, 2017

ad-m commented May 25, 2017

patricknelson commented May 25, 2017

reddraggone9 commented May 25, 2017 •

edited

Loading

patricknelson commented May 25, 2017

reddraggone9 commented May 25, 2017

reddraggone9 May 25, 2017 •

edited

Loading

chaosbot commented May 25, 2017

rudehn commented May 26, 2017

ad-m commented May 26, 2017

ad-m commented May 26, 2017

rudehn commented May 26, 2017 •

edited

Loading

Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates #250

Prevent m̶a̶l̶i̶c̶i̶o̶u̶s̶ invalid code updates #250

Conversation

ad-m commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented May 25, 2017

reddraggone9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ad-m commented May 25, 2017

PlasmaPower commented May 25, 2017

reddraggone9 May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricknelson commented May 25, 2017

amoffat commented May 25, 2017

ad-m commented May 25, 2017

ad-m commented May 25, 2017

reddraggone9 commented May 25, 2017

ad-m commented May 25, 2017

patricknelson commented May 25, 2017

reddraggone9 commented May 25, 2017 • edited Loading

patricknelson commented May 25, 2017

reddraggone9 commented May 25, 2017

reddraggone9 May 25, 2017 • edited Loading

Choose a reason for hiding this comment

chaosbot commented May 25, 2017

rudehn commented May 26, 2017

ad-m commented May 26, 2017

ad-m commented May 26, 2017

rudehn commented May 26, 2017 • edited Loading

reddraggone9 May 25, 2017 •

edited

Loading

reddraggone9 commented May 25, 2017 •

edited

Loading

reddraggone9 May 25, 2017 •

edited

Loading

rudehn commented May 26, 2017 •

edited

Loading