Skip to content

Commit 0116e20

Browse files
committed
fix(security): allow symlink creation, check path escape only during file writes (#381)
- Remove strict validation during symlink creation to allow legitimate symlinks to system paths - Keep path validation during file writes using canonicalFilePath() to resolve symbolic links - Remove symlinkTargetIsWithinTarget function to simplify security check logic - Fix over-blocking issue while preventing Zip Slip attacks Before fix: lib.so -> /usr/lib/xxx was incorrectly rejected After fix: Symlink creation succeeds, writing to system files via symlinks is blocke Bug:https://pms.uniontech.com/bug-view-356233.html
1 parent e3f6b70 commit 0116e20

4 files changed

Lines changed: 11 additions & 62 deletions

File tree

3rdparty/interface/common.cpp

Lines changed: 9 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -540,60 +540,32 @@ bool Common::findLnfsPath(const QString &target, Compare func)
540540
bool extractPathIsWithinTarget(const QString &extractRoot, const QString &absoluteDestPath)
541541
{
542542
const QFileInfo rootFi(extractRoot);
543-
const QString rootCanon = rootFi.canonicalFilePath();
543+
QString rootCanon = rootFi.canonicalFilePath();
544+
// 如果 canonical 失败,使用绝对路径作为后备
544545
if (rootCanon.isEmpty()) {
545-
return false;
546+
rootCanon = QDir(extractRoot).absolutePath();
547+
if (rootCanon.isEmpty()) {
548+
return false;
549+
}
546550
}
547551

548552
const QString destAbs = QFileInfo(absoluteDestPath).absoluteFilePath();
549553
QString path = destAbs;
550554

555+
// 向上遍历路径,解析符号链接
551556
while (true) {
552557
QFileInfo fi(path);
553558
if (fi.exists()) {
554559
const QString canon = fi.canonicalFilePath();
555560
if (canon.isEmpty()) {
556561
return false;
557562
}
563+
// 检查解析后的路径是否在解压目录内
558564
if (!canon.startsWith(rootCanon + QDir::separator()) && canon != rootCanon) {
559565
return false;
560566
}
561567
return true;
562568
}
563-
const QString parent = fi.path();
564-
if (parent == path || parent.isEmpty()) {
565-
break;
566-
}
567-
path = parent;
568-
}
569-
return rootFi.exists();
570-
}
571-
572-
bool symlinkTargetIsWithinTarget(const QString &extractRoot, const QString &symlinkFilePath, const QString &symlinkTarget)
573-
{
574-
const QFileInfo rootFi(extractRoot);
575-
const QString rootCanon = rootFi.canonicalFilePath();
576-
if (rootCanon.isEmpty()) {
577-
return false;
578-
}
579-
580-
if (symlinkTarget.isEmpty()) {
581-
return false;
582-
}
583-
584-
const QString linkParent = QFileInfo(symlinkFilePath).path();
585-
const QString resolved = QDir::cleanPath(QDir(linkParent).absoluteFilePath(symlinkTarget));
586-
QString path = resolved;
587-
588-
while (true) {
589-
QFileInfo fi(path);
590-
if (fi.exists()) {
591-
const QString canon = fi.canonicalFilePath();
592-
if (canon.isEmpty()) {
593-
return false;
594-
}
595-
return canon.startsWith(rootCanon + QDir::separator()) || canon == rootCanon;
596-
}
597569

598570
const QString parent = fi.path();
599571
if (parent == path || parent.isEmpty()) {
@@ -602,7 +574,7 @@ bool symlinkTargetIsWithinTarget(const QString &extractRoot, const QString &syml
602574
path = parent;
603575
}
604576

605-
return resolved.startsWith(rootCanon + QDir::separator()) || resolved == rootCanon;
577+
return rootFi.exists();
606578
}
607579

608580
bool IsMtpFileOrDirectory(QString path) noexcept {

3rdparty/interface/common.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,4 @@ bool IsMtpFileOrDirectory(QString path) noexcept;
6464
*/
6565
bool extractPathIsWithinTarget(const QString &extractRoot, const QString &absoluteDestPath);
6666

67-
/**
68-
* 解压安全:ZIP 内符号链接目标解析后须位于解压根目录内。
69-
*/
70-
bool symlinkTargetIsWithinTarget(const QString &extractRoot, const QString &symlinkFilePath, const QString &symlinkTarget);
71-
7267
#endif

3rdparty/libminizipplugin/libminizipplugin.cpp

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -311,19 +311,6 @@ ErrorType LibminizipPlugin::extractEntry(unzFile zipfile, unz_file_info file_inf
311311

312312
// 解压完整文件名(含路径)
313313
QString strDestFileName = options.strTargetPath + QDir::separator() + strFileName;
314-
315-
const QString cleanTargetPath = QDir::cleanPath(QDir(options.strTargetPath).absolutePath());
316-
const QString cleanDestPath = QDir::cleanPath(QDir(strDestFileName).absolutePath());
317-
if (!cleanDestPath.startsWith(cleanTargetPath + QDir::separator()) &&
318-
cleanDestPath != cleanTargetPath) {
319-
qInfo() << "Path traversal detected! Rejected path: " << strFileName;
320-
return ET_FileWriteError;
321-
}
322-
if (!extractPathIsWithinTarget(options.strTargetPath, strDestFileName)) {
323-
qInfo() << "Rejected path (symlink escape or out of root):" << strDestFileName;
324-
return ET_FileWriteError;
325-
}
326-
327314
QFile file(strDestFileName);
328315

329316
if (bIsDirectory) { // 文件夹

3rdparty/libzipplugin/libzipplugin.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -910,8 +910,9 @@ ErrorType LibzipPlugin::extractEntry(zip_t *archive, zip_int64_t index, const Ex
910910
return ET_FileWriteError;
911911
}
912912

913+
// 写入文件前检查路径是否通过符号链接逃逸
913914
if (!extractPathIsWithinTarget(options.strTargetPath, strDestFileName)) {
914-
qInfo() << "Rejected path (symlink escape or out of root):" << strDestFileName;
915+
qInfo() << "Rejected path (symlink escape detected):" << strDestFileName;
915916
return ET_FileWriteError;
916917
}
917918

@@ -971,12 +972,6 @@ ErrorType LibzipPlugin::extractEntry(zip_t *archive, zip_int64_t index, const Ex
971972
const auto readBytes = zip_fread(zipFile, buf, zip_uint64_t(READBYTES));
972973
if (readBytes > 0) {
973974
QString strBuf = QString(buf).toLocal8Bit();
974-
if (!symlinkTargetIsWithinTarget(options.strTargetPath, strDestFileName, strBuf)) {
975-
qInfo() << "Symlink target escapes extract root, rejected:" << strBuf;
976-
zip_fclose(zipFile);
977-
emit signalFileWriteErrorName(strBuf);
978-
return ET_FileWriteError;
979-
}
980975
if (QFile::link(strBuf, strDestFileName)) {
981976
qInfo() << "Symlink's created:" << buf << strFileName;
982977
} else {

0 commit comments

Comments
 (0)