Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add explanation of restic automation #1245

Merged
merged 2 commits into from
Sep 19, 2017
Merged

add explanation of restic automation #1245

merged 2 commits into from
Sep 19, 2017

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented Sep 16, 2017

every time i look at restic, i block on this and figured it may be
useful for others

this will at least partly address #533.

every time i look at restic, i block on this and figured it may be useful for others
@codecov-io
Copy link

codecov-io commented Sep 16, 2017

Codecov Report

Merging #1245 into master will decrease coverage by 5.62%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
- Coverage   52.45%   46.82%   -5.63%     
==========================================
  Files         131      131              
  Lines       12373    12373              
==========================================
- Hits         6490     5794     -696     
- Misses       5124     5875     +751     
+ Partials      759      704      -55
Impacted Files Coverage Δ
internal/backend/b2/b2.go 0% <0%> (-79.32%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-73.9%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-61.93%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-59.85%) ⬇️
internal/backend/swift/config.go 34.37% <0%> (-56.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c784a15...bcd1e45. Read the comment docs.

@@ -104,3 +104,7 @@

# Output file base name for HTML help builder.
htmlhelp_basename = 'resticdoc'

extlinks = {
'issue': ('https://github.com/restic/restic/issues/%s', '#'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't know that this is possible. Is it compatible with readthedocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is.

doc/faq.rst Outdated
How can I specify encryption passwords automatically?
-----------------------------------------------------

When you run ``restict create``, you need to enter the passphrase on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a typo: s/restict/restic/, and I suspect you mean restic backup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, confused with borg, sorry, will reword.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

.. important:: Be careful how you set the environment; using the env
command, a `system()` call or using inline shell
scripts (e.g. `RESTIC_PASSWORD=password borg ...`)
might expose the credentials in the process list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the case, are you sure? How does that work? Do you have a sample for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i must admit i coped this verbatim from the borg warning. but it fits with my experience. e.g.

$ env FOO=secretkey sleep 1 & sudo -u nobody ps axfue | grep -q FOO=secretkey && echo oops. secret leaked.
[2] 18143
oops. secret leaked.

calls with system("FOO=secret bar") will also be a problem, as this will call sh -c "FOO=secret bar" which will show up in the process list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Your example does not work: The grep -q returns exit code 0 because it sees itself in the process list, not the env:

$ env FOO=secretkey sleep 10 & sudo -u nobody ps axfue | grep FOO
[1] 14446
fd0      14448  0.0  0.0  12432  2420 pts/8    S+   20:37   0:00          \_ grep FOO

The program env will have the secret value in its command line, which is world readable, so I agree that in principle this problem exists. But an instance of env is extremely short-lived: It just sets the environment variables from the arguments, then runs exceve() (or something similar) and the old process is replaced by the new one, and the entry in /proc/PID/cmdline is also replaced. At least that's what I just verified with gdb.

The case with system() I just tried to reproduce with ruby/irb, and for system("FOO=x sleep 500") I was unable to see the intermediate sh -c: The sleep binary was a direct child to the irb process.

Please don't get me wrong: I appreciate your contribution and the added FAQ entry, but I'd like to make sure that this warning is really accurate and we don't scare users needlessly.

Copy link
Contributor Author

@anarcat anarcat Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can't speak about ruby as i'm less familiar with it, but i can reproduce with a Python system() call, however. the faulty script:

import os
os.system("RESTIC_PASSWORD=secretkey sleep 1")

the sniffer that catches it:

#!/usr/bin/perl -w

use Proc::ProcessTable;

$| = 1;

my $i = 0;
my $start = time();
while (1) {
  $i++;
  my $delta = time() - $start;
  if ($delta > 0) {
    printf("\r%d loops per second", ($i/$delta));
    $start = time();
    $i = 0;
  }
  my $t = new Proc::ProcessTable;
  foreach my $p ( @{$t->table} ){
    if ($p->cmndline =~ m/RESTIC_PASSWORD=(\w+)/) {
      printf("\npass: $1, cmd: %s\n", $p->cmndline);
    }
  }
}

output of the sniffer:

$ ./sniff-cli.pl
66 loops per second

pass: secretkey, cmd: sh -c RESTIC_PASSWORD=secretkey sleep 1

pass: secretkey, cmd: sh -c RESTIC_PASSWORD=secretkey sleep 1

[... repeated about 80 times ...]
63 loops per second^C

the workaround, in Python, is of course to set os.environ directly and call the subprocess module with shell=False. i think the above covers both the system() and "inline shell script" descriptions.

i must admit i am having trouble reproducing with env: it's pretty fast at doing that execve, as you aptly found out, so it's hard to catch it in the act. my attack script can only leverage about 60Hz (e.g. 60 samples of the process list per second) which is very low - i suspect there are more efficient ways of doing this that would catch env in the act because it is passing through the process list. "extremely short lived" is not good enough for something that leaks through the process list, in my opinion. the risk is low, but the impact is so catastrophic that i think such practices should be discouraged.

note the sniffer is in perl for historical reasons, but I also wrote a Python version to see if it would be faster - using some bypassing tricks, i can go up to about 160Hz, but i still can't hit the env calls:

#!/usr/bin/python3

import datetime
import psutil

t = datetime.datetime.now()
i = 0
while True:
    i += 1
    delta = (datetime.datetime.now() - t).seconds
    if delta:
        print("\r%d loops per second" % (i/int(delta)))
        t = datetime.datetime.now()
        i = 0
    for proc in psutil.pids():
        try:
            cmdline = psutil._psplatform.Process(proc).cmdline()
            d = [x for x in cmdline if 'RESTIC_PASSWORD' in x]
            if d:
                print(cmdline)
        except psutil.NoSuchProcess:
            pass

Copy link
Contributor Author

@anarcat anarcat Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i obsessed over this a little more and wrote this dumb C program that achieves better performance:

#include <proc/readproc.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

int main(int argc, char *argv[]){
  pid_t* pidlist;
  int flags;
  PROCTAB* ptp;
  static proc_t buf;
  int i, j;
  time_t t0, t1;
  double diff;
  flags = PROC_FILLCOM;
  time(&t0);
  for (i = 0; 1; i++) {
    time(&t1);
    diff = (int) difftime(t1, t0);
    if (diff > 0) {
      printf("\r%0.0f loops per second", (i/diff));
      fflush(stdout);
      time(&t0);
      i = 0;
    }
    ptp = openproc(flags, pidlist);
    while (readproc(ptp, &buf)) {
      for (j = 0; buf.cmdline && buf.cmdline[j]; j++) {
        if (strstr(buf.cmdline[j], "RESTIC_PASSWORD")) {
          printf("\n%s\n", buf.cmdline[j]);
          exit(0);
        }
      }
    }
    closeproc(ptp);
  }
}

this gets between 700 and 900 Hz (ie. loop iterations per second), and it does catch env, if I run env in a tight loop like this:

while true; do
    /bin/env RESTIC_PASSWORD=secret true
done

now. i know what you'll say: this is unfair to poor env, we're mistreating it totally. but consider this: even if I run the loop like this:

while true; do
    /bin/env RESTIC_PASSWORD=secret sleep 10
done

... it eventually finds it. on my first try, when trying this, it found it in 3 seconds (so on the first try!):

$ time ./sniff-cli
1001 loops per second
RESTIC_PASSWORD=secret
0.43user 2.07system 0:02.52elapsed 99%CPU (0avgtext+0avgdata 2608maxresident)k
0inputs+0outputs (0major+143minor)pagefaults 0swaps

pure coincidence of course. in general, it's going to take a few attempts to get it. for example, on my second try, it took 53 seconds to catch the env call (so about 5 env calls)... then on the third try, 43 seconds (so 4), then again, 147 seconds (so 14). so i'd say i've got around 10-20% chance of hitting the password, but i don't have a statistically valid sample. it's a very narrow margin, but it's non-zero. a more elaborate experiment could be ran to extract more reliable stats, of course.

now, arguably, this attack require arbitrary code execution from another user, using a dedicated CPU rolling full speed, something which could be detected and worked around, but i think this is something we should be worried about for something as critical as a crypto passphrase. just don't shove this on the commandline. environment is okay, as the comment says, but commandline is not.

(hopefully, this is the last time I need to explain this to anyone - every time i need to build a more optimized version of the code as CPUs speed up... i guess that, eventually, i'll just lose that battle completely :p)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, and to complete our little programming language tour, the motivated reader may want to write their own version in golang, with coroutines coordinating the listing and inspection of the cmdline field to max out CPUs.

in the end, on linux, the best we can do is iterate real hard on /proc - it's what ps and readproc(2) is doing anyways... anything better would need root/kernel module access, at which point the game is over anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You convinced me to keep this paragraph as is, great work! ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about saving password in a file with rights 600 owned by user which runs the restic process? Doesn't it completely eliminate the said problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donat-b i added a comment about safe file permissions. it does resolve the problem, which is why we warn about passing environment on the commandline (and not a password file).

now what i would like to see is the possibility of encrypting said password file with GPG or some other external command. that would neatly solve #533 at low implementation cost... but that belongs in that other thread i guess. :)

Copy link
Member

@fd0 fd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fd0 fd0 merged commit bcd1e45 into restic:master Sep 19, 2017
fd0 added a commit that referenced this pull request Sep 19, 2017
add explanation of restic automation
@anarcat anarcat deleted the faq branch September 20, 2017 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants