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

imagewriter: Force permissions on settings to 600 #772

Open
wants to merge 2 commits into
base: qml
Choose a base branch
from

Conversation

tdewey-rpi
Copy link
Collaborator

Use this mechanism to replace an older scheme that fully read and then fully wrote the file.

Resolves #770

Use this mechanism to replace an older scheme that fully read and then fully wrote the file.

Resolves raspberrypi#770
@tdewey-rpi tdewey-rpi requested review from maxnet and cillian64 January 5, 2024 15:54
@maxnet
Copy link
Collaborator

maxnet commented Jan 5, 2024

Upon information and belief QSettings does not create any file on disk until you either call sync() or the QSettings object is destructed.

You are now correcting permission on Imager's start (ImageWriter's constructor), if the .conf file exists at that time.

So let's imagine that I have never used Imager before (so .conf does not exist yet), use it to write the single SD card I have, and then won't be using Imager for a while.
What permissions will the .conf in my home directory after using Imager that only time have?

Also be aware that on Windows the fileName() returned will be a registry key.
While you are probably saved by your file existence check, you may wish to #ifndef Q_OS_WIN the code just in case.

@tdewey-rpi
Copy link
Collaborator Author

A fair comment - and I'll see if we can compel QSettings into creating a file with the appropriate permissions.

@maxnet
Copy link
Collaborator

maxnet commented Jan 5, 2024

A fair comment - and I'll see if we can compel QSettings into creating a file with the appropriate permissions.

Think that it is not doing that, may actually be a long term Qt bug.

https://github.com/qt/qtbase/blob/dev/src/corelib/io/qsettings.cpp#L1509-L1513

Don't think the "fileInfo.permissions() |" part is supposed to be there.
But getting such things fixed upstream may be problematic. Change in behavior, etc.

@tdewey-rpi
Copy link
Collaborator Author

Agreed - this looks like an upstream issue. I've just had a test of the scenario you describe - and sure enough after a sync() we get a 644 file.

The only comprehensive fix I can see with the QSettings API is to register our own read & write functions, and then re-implement the body of the QSettings file reader and writer. Deeply undesirable, but might be the only way to coerce it into doing the right thing.

https://doc.qt.io/qt-6/qsettings.html#registerFormat

@maxnet
Copy link
Collaborator

maxnet commented Jan 5, 2024

Chances are you may also be able to (ab)use the umask() syscall to change default file creation mode.

As in put somewhere at the top of main()

#ifdef Q_OS_LINUX
   /* set umask to octal 077 resulting in files being created rw------- by default */
   umask(0077);
#endif

@tdewey-rpi
Copy link
Collaborator Author

@cillian64 If you have bandwidth, might be worth looking at this horror as mentioned earlier. Ignoring that it's a TOC/TOU problem, in the near-term we can probably address it by bunding a setPermissions(...) call on the back of every flush().

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.

[BUG] Insecure permissions for configuration file holding hashed password & plaintext WiFi password
2 participants