Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix a possible security issue
- script output might have been stored to paths elsewhere
- Thanks to G.C. for reporting
  • Loading branch information
gzotti committed Mar 4, 2023
1 parent 2a2f8a1 commit 1261f74
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 7 deletions.
12 changes: 6 additions & 6 deletions src/scripting/StelScriptOutput.cpp
Expand Up @@ -56,15 +56,15 @@ void StelScriptOutput::reset(void)
void StelScriptOutput::saveOutputAs(const QString &name)
{
QFile asFile;
QFileInfo outputInfo(outputFile);
QDir dir=outputInfo.dir(); // will hold complete dirname
QFileInfo newFileNameInfo(name);
const QFileInfo outputInfo(outputFile);
const QDir dir=outputInfo.dir(); // will hold complete dirname
const QFileInfo newFileNameInfo(name);

bool okToSaveToAbsolutePath=StelApp::getInstance().getSettings()->value("scripts/flag_script_allow_write_absolute_path", false).toBool();
const bool okToSaveToAbsolutePath=StelApp::getInstance().getSettings()->value("scripts/flag_script_allow_write_absolute_path", false).toBool();

if (!okToSaveToAbsolutePath && (newFileNameInfo.isAbsolute()))
if (!okToSaveToAbsolutePath && ((newFileNameInfo.isAbsolute() || (name.contains(".."))))) // The last condition may include dangerous/malicious paths

This comment has been minimized.

Copy link
@10110111

10110111 Mar 4, 2023

Contributor

Why so many parentheses? The condition as expressed now is basically x && ((y || (z)))) — this is unreadable! It could be just as well reformatted as x && (y || z).

{
qWarning() << "SCRIPTING CONFIGURATION ISSUE: You are trying to save to an absolute pathname.";
qWarning() << "SCRIPTING CONFIGURATION ISSUE: You are trying to save to an absolute pathname or move up in directories.";
qWarning() << " To enable this, edit config.ini and set [scripts]/flag_script_allow_write_absolute_path=true";
asFile.setFileName(dir.absolutePath() + "/" + newFileNameInfo.fileName());
qWarning() << " Storing to " << asFile.fileName() << " instead";
Expand Down
3 changes: 2 additions & 1 deletion src/scripting/StelScriptOutput.hpp
Expand Up @@ -41,12 +41,13 @@ class StelScriptOutput
static void writeLog(QString msg);

//! Reset file, i.e., empty it. This may be useful to have repetitive output which may be read by other programs.
//! Normally you would call saveOutputAs(...), then reset().
static void reset(void);

//! Save to new file, re-create output file.
//! This allows reading of results on Windows, where otherwise reading programs cannot access files opened for writing by Stellarium.
//! @param name new filename. If this is not an absolute path, it will be created in the same directory as output.txt
//! @note For storing to absolute path names, set [scripts]/flag_script_allow_write_absolute_path=true.
//! @note For storing to absolute path names or paths containing directory navigation (".."), set [scripts]/flag_script_allow_write_absolute_path=true.
//! Normally you would call saveOutputAs(...), then reset().
static void saveOutputAs(const QString& name);

Expand Down

0 comments on commit 1261f74

Please sign in to comment.