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

REPL history file should not be world-readable #3392

Closed
XeCycle opened this issue Oct 16, 2015 · 13 comments
Closed

REPL history file should not be world-readable #3392

XeCycle opened this issue Oct 16, 2015 · 13 comments
Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem. security Issues and PRs related to security.

Comments

@XeCycle
Copy link
Contributor

XeCycle commented Oct 16, 2015

This is what we normally do for privacy.

~ $ ll .*history
-rw------- 1 xecycle xecycle 1.1K Sep 23 12:02 .bash_history
-rw------- 1 xecycle xecycle  326 Jun 23 16:30 .gnuplot_history
-rw------- 1 xecycle xecycle  67K Sep 18 14:32 .mysql_history
-rw-r--r-- 1 xecycle xecycle    0 Oct 16 13:17 .node_repl_history
-rw------- 1 xecycle xecycle  20K Oct 12 17:18 .psql_history
-rw------- 1 xecycle xecycle   15 Mar 11  2015 .zcalc_history
@Trott Trott added repl Issues and PRs related to the REPL subsystem. good first issue Issues that are suitable for first-time contributors. labels Oct 16, 2015
@Trott
Copy link
Member

Trott commented Oct 16, 2015

FWIW, irb also does not enforce a restrictive umask.

$ rm .irb-history 
$ irb
2.2.0 :001 > print "hello world"
hello world => nil 
2.2.0 :002 > 
$ ls -l .irb-history 
-rw-r--r--  1 trott  trott  20 Oct 15 22:29 .irb-history
$

I agree, though, that it ought to default to umask 077.

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

This is trivial to fix, and I am working on it. Do we want to chmod existing files?

@rvagg
Copy link
Member

rvagg commented Oct 16, 2015

probably only do it on file creation

anyone know what the .bash_history policy is here? that might be worth copying.

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

Bash does not chmod. So I am going that way.

~ $ ll .bash_history 
-rw------- 1 xecycle xecycle 1.1K Sep 23 12:02 .bash_history
~ $ chmod og+r .bash_history 
~ $ bash
[xecycle@xcws1 ~]$ exit
~ $ ll .bash_history 
-rw-r--r-- 1 xecycle xecycle 1.1K Sep 23 12:02 .bash_history

@targos
Copy link
Member

targos commented Oct 16, 2015

zsh creates it like that:
-rw------- 1 mzasso mzasso 37 16 oct. 09:07 .zsh_history

@Trott
Copy link
Member

Trott commented Oct 16, 2015

@XeCycle The fix is trivial (a one-line change in lib/internal/repl.js) but the test is a little tricky. Here's what I came up with for a test (to be put in a file with a name along the lines of test/parallel/test-repl-history.js)

'use strict';

const common = require('../common');
const assert = require('assert');
const path = require('path');
const fs = require('fs');
const repl = require('internal/repl');

// Invoking the REPL should create a repl history file at the specified path
// and mode 600.

common.refreshTmpDir();
const replHistoryPath = path.join(common.tmpDir, 'repl_history');

const checkResults = common.mustCall(function(err, r) {
  if (err)
    throw err;
  r.input.end();
  const stat = fs.statSync(replHistoryPath);
  const mode = '0' + (stat.mode & parseInt('777', 8)).toString(8);
  assert.strictEqual(mode, '0600', 'REPL history file should be mode 0600');
});

repl.createInternalRepl(
  {NODE_REPL_HISTORY: replHistoryPath},
  {terminal: true},
  checkResults
);

@Trott
Copy link
Member

Trott commented Oct 16, 2015

While writing the above test, I noticed that I do not need to pass --expose-internals to require internal/repl. Is that a bug?

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

@Trott I am attempting to test for it on process beforeExit. Not very sure about everything, but I think it better to avoid testing on internal interface.

@Trott
Copy link
Member

Trott commented Oct 16, 2015

I ran into issues going that route. I think it was because of the way a history file is not created if there is no tty. There may very well be a way around that issue (or whatever weirdness I was running into). So, yes, all things being equal, if you can make it work, testing against the public interface would probably be better.

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

I see, the problem is not about terminals. Only internal/repl creates a history file, the public repl does not.

@Trott
Copy link
Member

Trott commented Oct 16, 2015

Right, so you'd need to use spawn() or similar to fire up another node instance and have it enter the REPL the same way a user would. And that's where the terminal stuff (or something like it) comes up. I gave up on that route and came up with the above test instead. (It's yours to use or ignore as you see fit!)

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

The way looks to me like adding it in test/addons a directory for this and call openpty from C++... Is tty going to have an openpty?

@XeCycle
Copy link
Contributor Author

XeCycle commented Oct 16, 2015

Oh, openpty is not available on Windows. I give up, will use your test code, adding a dumb stream so that stdout is also silent.

Trott pushed a commit to Trott/io.js that referenced this issue Oct 20, 2015
@ChALkeR ChALkeR added the security Issues and PRs related to security. label May 3, 2016
XeCycle added a commit to XeCycle/node that referenced this issue May 4, 2016
evanlucas pushed a commit that referenced this issue May 17, 2016
Set the mode bits on the history file to 0o600 instead of leaving it
unspecified, which resulted in 0o755 on Unices.

Test code mostly written by Trott:
#3392 (comment).

PR-URL: #3394
Fixes: #3392
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 1, 2016
Set the mode bits on the history file to 0o600 instead of leaving it
unspecified, which resulted in 0o755 on Unices.

Test code mostly written by Trott:
#3392 (comment).

PR-URL: #3394
Fixes: #3392
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 23, 2016
Set the mode bits on the history file to 0o600 instead of leaving it
unspecified, which resulted in 0o755 on Unices.

Test code mostly written by Trott:
#3392 (comment).

PR-URL: #3394
Fixes: #3392
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 24, 2016
Set the mode bits on the history file to 0o600 instead of leaving it
unspecified, which resulted in 0o755 on Unices.

Test code mostly written by Trott:
#3392 (comment).

PR-URL: #3394
Fixes: #3392
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem. security Issues and PRs related to security.
Projects
None yet
Development

No branches or pull requests

5 participants