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

Access Permission Issues for Datastore saved files. - Electron #514

Closed
sinumohan opened this issue Jun 13, 2017 · 7 comments · May be fixed by #549
Closed

Access Permission Issues for Datastore saved files. - Electron #514

sinumohan opened this issue Jun 13, 2017 · 7 comments · May be fixed by #549

Comments

@sinumohan
Copy link

sinumohan commented Jun 13, 2017

I am using NeDB as a local storage for my electron app. Everything works fine except when I update my app , I want the data to be saved locally . But the new version cannot access data from files written by my old version . it throws an EACCESS : permission denied Exception.

var db = new Datastore({ filename:"PATH_TO_FILE",autoload:true});

@JamesMGreene
Copy link
Contributor

I can't see how this has anything to do with NeDB. I would suggest asking within the Electron community how others are already handling this today.

@p3nGu1nZz
Copy link

try changing the permissions on the file

fs.chmod(fileName, 0666, (error) => {
  console.log('Changed file permissions');
});

another way is to make sure you run the app as admin or with sudo. gl`

@mbasunov
Copy link

Initially incorrect file access rights should be fixed manually.
Proposed pull request adds NeDB control of file access mode.

@louischatriot
Copy link
Owner

Indeed hard for me to see this as relating to NeDB given this is a file permission issue, likely due to config switch between Electron versions.

Also not sure I see value in having NeDB manage datafile permissions vs simply the developer?

@mbasunov
Copy link

It is insecure to manipulate file access righs AFTER creation and writing file contents.
Access righs should be set BEFORE creating a file or any attacker could listen to FS changes and to open a file before access righs were "fixed".

NeDB uses default NodeJS methods fs.open and fs.writeFile and asynchronous operation.
All these disallows me (as a developer and as an OS administrator) to control over file mode of NeDB files.
I want more secure files to be created with mode 600 and less secure file to be created with mode 640/660.
But you're simply arguing that NeDB shouldn't manage datafile permissions.
Why?
Why OS call to "open" makes this possible and you're revoking these righs from me?

$ umask
0000
$ node aa.js ; ls -l aa.nedb
-rw-rw-rw-. 1 xx xx 0 Mar 26 22:42 aa.nedb
const NeDB = require('nedb')
const fs = require('fs')
const msk = process.umask(0o060);
// enable autoload for the file to be created almost immediately
const db = new NeDB({filename: 'aa.nedb', autoload: true});
process.umask(msk);

Just refer to my pull request #549 which makes possible to specify file access mode and pass it to fs.open and fs.writeFile functions making file manipulation more secure and controllable way.

@louischatriot
Copy link
Owner

I saw the PR indeed but I still do not think permissions should be managed by NeDB - if you are worried about security to the point of wanting to mitigate the possibility of an attacker listening to FS changes I'm not sure NeDB is the right choice. Not that it has vulnerabilities that I know of, but given the intended usecase I did not put extra care on security.

@mbasunov
Copy link

Nice to hear from you that you're not worrying about security.
Each your internal call to "crashSafeWriteFile" would reset my permissions set on target file to insecure.
Anyway, without modification of NeDB I'm ABSOLUTELY unable to control over file permissions, because NeDB overwrite it asynchronously without any notification/callback.

The simpliest way to properly handle file permissions is to pass it to NodeJS file operations via NeDB constructor.
Otherwise it is unsolvable.

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 a pull request may close this issue.

5 participants