Skip to content

Commit ec33867

Browse files
authored
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 7fc5ed3 commit ec33867

File tree

4 files changed

+11
-62
lines changed

4 files changed

+11
-62
lines changed

3rdparty/interface/common.cpp

Lines changed: 9 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -521,60 +521,32 @@ bool Common::findLnfsPath(const QString &target, Compare func)
521521
bool extractPathIsWithinTarget(const QString &extractRoot, const QString &absoluteDestPath)
522522
{
523523
const QFileInfo rootFi(extractRoot);
524-
const QString rootCanon = rootFi.canonicalFilePath();
524+
QString rootCanon = rootFi.canonicalFilePath();
525+
// 如果 canonical 失败,使用绝对路径作为后备
525526
if (rootCanon.isEmpty()) {
526-
return false;
527+
rootCanon = QDir(extractRoot).absolutePath();
528+
if (rootCanon.isEmpty()) {
529+
return false;
530+
}
527531
}
528532

529533
const QString destAbs = QFileInfo(absoluteDestPath).absoluteFilePath();
530534
QString path = destAbs;
531535

536+
// 向上遍历路径,解析符号链接
532537
while (true) {
533538
QFileInfo fi(path);
534539
if (fi.exists()) {
535540
const QString canon = fi.canonicalFilePath();
536541
if (canon.isEmpty()) {
537542
return false;
538543
}
544+
// 检查解析后的路径是否在解压目录内
539545
if (!canon.startsWith(rootCanon + QDir::separator()) && canon != rootCanon) {
540546
return false;
541547
}
542548
return true;
543549
}
544-
const QString parent = fi.path();
545-
if (parent == path || parent.isEmpty()) {
546-
break;
547-
}
548-
path = parent;
549-
}
550-
return rootFi.exists();
551-
}
552-
553-
bool symlinkTargetIsWithinTarget(const QString &extractRoot, const QString &symlinkFilePath, const QString &symlinkTarget)
554-
{
555-
const QFileInfo rootFi(extractRoot);
556-
const QString rootCanon = rootFi.canonicalFilePath();
557-
if (rootCanon.isEmpty()) {
558-
return false;
559-
}
560-
561-
if (symlinkTarget.isEmpty()) {
562-
return false;
563-
}
564-
565-
const QString linkParent = QFileInfo(symlinkFilePath).path();
566-
const QString resolved = QDir::cleanPath(QDir(linkParent).absoluteFilePath(symlinkTarget));
567-
QString path = resolved;
568-
569-
while (true) {
570-
QFileInfo fi(path);
571-
if (fi.exists()) {
572-
const QString canon = fi.canonicalFilePath();
573-
if (canon.isEmpty()) {
574-
return false;
575-
}
576-
return canon.startsWith(rootCanon + QDir::separator()) || canon == rootCanon;
577-
}
578550

579551
const QString parent = fi.path();
580552
if (parent == path || parent.isEmpty()) {
@@ -583,7 +555,7 @@ bool symlinkTargetIsWithinTarget(const QString &extractRoot, const QString &syml
583555
path = parent;
584556
}
585557

586-
return resolved.startsWith(rootCanon + QDir::separator()) || resolved == rootCanon;
558+
return rootFi.exists();
587559
}
588560

589561
bool IsMtpFileOrDirectory(QString path) noexcept {

3rdparty/interface/common.h

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

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

3rdparty/libminizipplugin/libminizipplugin.cpp

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

302302
// 解压完整文件名(含路径)
303303
QString strDestFileName = options.strTargetPath + QDir::separator() + strFileName;
304-
305-
const QString cleanTargetPath = QDir::cleanPath(QDir(options.strTargetPath).absolutePath());
306-
const QString cleanDestPath = QDir::cleanPath(QDir(strDestFileName).absolutePath());
307-
if (!cleanDestPath.startsWith(cleanTargetPath + QDir::separator()) &&
308-
cleanDestPath != cleanTargetPath) {
309-
qInfo() << "Path traversal detected! Rejected path: " << strFileName;
310-
return ET_FileWriteError;
311-
}
312-
if (!extractPathIsWithinTarget(options.strTargetPath, strDestFileName)) {
313-
qInfo() << "Rejected path (symlink escape or out of root):" << strDestFileName;
314-
return ET_FileWriteError;
315-
}
316-
317304
QFile file(strDestFileName);
318305

319306
if (bIsDirectory) { // 文件夹

3rdparty/libzipplugin/libzipplugin.cpp

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

898+
// 写入文件前检查路径是否通过符号链接逃逸
898899
if (!extractPathIsWithinTarget(options.strTargetPath, strDestFileName)) {
899-
qInfo() << "Rejected path (symlink escape or out of root):" << strDestFileName;
900+
qInfo() << "Rejected path (symlink escape detected):" << strDestFileName;
900901
return ET_FileWriteError;
901902
}
902903

@@ -956,12 +957,6 @@ ErrorType LibzipPlugin::extractEntry(zip_t *archive, zip_int64_t index, const Ex
956957
const auto readBytes = zip_fread(zipFile, buf, zip_uint64_t(READBYTES));
957958
if (readBytes > 0) {
958959
QString strBuf = QString(buf).toLocal8Bit();
959-
if (!symlinkTargetIsWithinTarget(options.strTargetPath, strDestFileName, strBuf)) {
960-
qInfo() << "Symlink target escapes extract root, rejected:" << strBuf;
961-
zip_fclose(zipFile);
962-
emit signalFileWriteErrorName(strBuf);
963-
return ET_FileWriteError;
964-
}
965960
if (QFile::link(strBuf, strDestFileName)) {
966961
qInfo() << "Symlink's created:" << buf << strFileName;
967962
} else {

0 commit comments

Comments
 (0)