feat: add layer signature extraction support#1706
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces signature extraction functionality for layer packages, integrating it into the package installation process with overlay support, transaction rollbacks, and updated error handling. It also adds corresponding unit tests. The reviewer identified several critical issues, including a potential infinite loop on EOF during signature reading, unhandled JSON parsing exceptions that could crash the daemon, a security risk of arbitrary file writes when extracting the tarball as root, silently ignored extraction errors, and a missing rollback transaction in installRefModule upon verification failure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| auto readBytes = ::read(selfFd, buf.data(), bytesToRead); | ||
| if (readBytes == -1) { | ||
| if (errno == EINTR) { | ||
| errno = 0; | ||
| continue; | ||
| } | ||
| return LINGLONG_ERR( | ||
| fmt::format("read sign data error: {}", common::error::errorString(errno))); | ||
| } |
There was a problem hiding this comment.
If the layer file is truncated or corrupted such that the actual signature data is smaller than signSize, ::read will return 0 (EOF). In this case, writeBytes will also be 0, writeBytes != readBytes will be false, and totalBytes will not decrease. This results in an infinite loop that consumes 100% CPU.
To fix this, explicitly check for readBytes == 0 and return an error.
auto readBytes = ::read(selfFd, buf.data(), bytesToRead);
if (readBytes == 0) {
return LINGLONG_ERR("unexpected EOF while reading signature data");
}
if (readBytes == -1) {
if (errno == EINTR) {
errno = 0;
continue;
}
return LINGLONG_ERR(
fmt::format("read sign data error: {}", common::error::errorString(errno)));
}| auto metaRawData = file.read(metaLen); | ||
| auto metaJson = nlohmann::json::parse(metaRawData.toStdString()); | ||
| auto erofsSize = metaJson.value("erofs_size", 0ULL); | ||
| auto signSize = metaJson.value("sign_size", 0ULL); |
There was a problem hiding this comment.
nlohmann::json::parse throws a parse_error exception if the input is not valid JSON. Since metaRawData comes from an external file on disk, a corrupted or maliciously crafted layer file could crash the daemon.
Use the non-throwing overload of parse by passing false for allow_exceptions, and verify that the parsed JSON is indeed an object before accessing its keys.
| auto metaRawData = file.read(metaLen); | |
| auto metaJson = nlohmann::json::parse(metaRawData.toStdString()); | |
| auto erofsSize = metaJson.value("erofs_size", 0ULL); | |
| auto signSize = metaJson.value("sign_size", 0ULL); | |
| auto metaRawData = file.read(metaLen); | |
| auto metaJson = nlohmann::json::parse(metaRawData.toStdString(), nullptr, false); | |
| if (metaJson.is_discarded()) { | |
| return LINGLONG_ERR("failed to parse metadata JSON"); | |
| } | |
| if (!metaJson.is_object()) { | |
| return LINGLONG_ERR("metadata JSON is not an object"); | |
| } | |
| auto erofsSize = metaJson.value("erofs_size", 0ULL); | |
| auto signSize = metaJson.value("sign_size", 0ULL); |
| } | ||
| tarFd = -1; | ||
|
|
||
| auto ret = utils::Cmd("tar").exec({ "-xf", tarFile.string(), "-C", destination.string() }); |
There was a problem hiding this comment.
Extracting an untrusted tarball (sign.tar) as root without restricting permissions or ownership poses a security risk. A malicious layer could include setuid binaries or attempt directory traversal.
To mitigate this, pass --no-same-owner and --no-same-permissions to the tar command to prevent extracting files with arbitrary owners or setuid permissions.
| auto ret = utils::Cmd("tar").exec({ "-xf", tarFile.string(), "-C", destination.string() }); | |
| auto ret = utils::Cmd("tar").exec({ "-xf", tarFile.string(), "--no-same-owner", "--no-same-permissions", "-C", destination.string() }); |
|
pre-commit.ci autofix |
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
1. Implement extractSignData in LayerPackager to extract signature data from layer files 2. Modify installFromLayer to extract sign data and pass overlays to repo->importLayerDir 3. Add transaction rollback for importLayerDir in installFromLayer and installRef 4. Fix error handling in executePostInstallHooks: return error instead of logging 5. Add unit tests for extractSignData with signed and unsigned layers Log: Added signature extraction for layer installation, enabling signature verification and overlay application. Influence: 1. Test installation of layers with valid signatures 2. Test installation of layers without signatures (should succeed) 3. Test installation with corrupted or missing signature metadata 4. Verify that signature overlay is correctly extracted and placed 5. Test rollback on failure during signature extraction 6. Verify error when post-install hooks fail due to signature mismatch 7. Test cleanup of temporary signature files after installation
deepin pr auto review★ 总体评分：45分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 utils::error::Result<void> LayerPackager::extractSignData(LayerFile &file,
const std::filesystem::path &signDir)
{
LINGLONG_TRACE("extract sign data from layer");
const auto &number = magicNumber();
file.seek(number.size());
QDataStream metaLenStream(&file);
metaLenStream.setByteOrder(QDataStream::LittleEndian);
quint32 metaLen = 0;
metaLenStream >> metaLen;
auto metaRawData = file.read(metaLen);
auto metaJson = nlohmann::json::parse(metaRawData.toStdString());
auto erofsSize = metaJson.value("erofs_size", 0ULL);
auto signSize = metaJson.value("sign_size", 0ULL);
if (signSize == 0) {
return LINGLONG_OK;
}
// 修复安全漏洞1：防止整数溢出
auto baseOffset = number.size() + sizeof(quint32) + metaLen;
if (erofsSize > static_cast<uint64_t>(std::numeric_limits<int64_t>::max()) - baseOffset) {
return LINGLONG_ERR("erofs_size is too large, potential integer overflow");
}
auto signOffset = static_cast<int64_t>(baseOffset + erofsSize);
std::error_code ec;
auto destination = signDir / "entries" / "share" / "deepin-elf-verify" / ".elfsign";
if (!std::filesystem::create_directories(destination, ec) && ec) {
return LINGLONG_ERR(ec.message().c_str());
}
auto tarFile = destination / "sign.tar";
auto tarFd = ::open(tarFile.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0644);
if (tarFd == -1) {
return LINGLONG_ERR(
fmt::format("open {} failed: {}", tarFile.string(), common::error::errorString(errno)));
}
auto removeTar = utils::finally::finally([&tarFd, &tarFile] {
if (tarFd != -1) {
::close(tarFd);
}
std::error_code ec;
if (!std::filesystem::remove(tarFile, ec) && ec) {
LogW("failed to remove {}: {}", tarFile.string(), ec.message());
}
});
file.seek(signOffset);
auto selfFd = file.handle();
auto totalBytes = signSize;
std::array<unsigned char, 4096> buf{};
while (totalBytes > 0) {
auto bytesToRead = totalBytes > buf.size() ? buf.size() : totalBytes;
auto readBytes = ::read(selfFd, buf.data(), bytesToRead);
if (readBytes == -1) {
if (errno == EINTR) {
errno = 0;
continue;
}
return LINGLONG_ERR(
fmt::format("read sign data error: {}", common::error::errorString(errno)));
}
// 修复语法逻辑错误：处理 EOF 防止死循环
if (readBytes == 0) {
return LINGLONG_ERR("read sign data error: unexpected EOF");
}
while (true) {
auto writeBytes = ::write(tarFd, buf.data(), readBytes);
if (writeBytes == -1) {
if (errno == EINTR) {
errno = 0;
continue;
}
return LINGLONG_ERR(
fmt::format("write sign.tar error: {}", common::error::errorString(errno)));
}
if (writeBytes != readBytes) {
return LINGLONG_ERR("write sign.tar failed: byte mismatch");
}
totalBytes -= writeBytes;
break;
}
}
if (::fsync(tarFd) == -1) {
return LINGLONG_ERR(
fmt::format("fsync sign.tar error: {}", common::error::errorString(errno)));
}
if (::close(tarFd) == -1) {
tarFd = -1;
return LINGLONG_ERR(
fmt::format("failed to close tar: {}", common::error::errorString(errno)));
}
tarFd = -1;
// 修复安全漏洞2：限制 tar 解压行为，防止路径遍历
auto ret = utils::Cmd("tar").exec({ "-xf", tarFile.string(), "-C", destination.string(),
"--no-absolute-filenames", "--no-same-permissions", "--no-same-owner" });
if (!ret) {
return LINGLONG_ERR(ret);
}
return LINGLONG_OK;
} |
|
@dengbo11: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
2 participants
Log: Added signature extraction for layer installation, enabling signature verification and overlay application.
Influence: