Repository navigation
sync: from linuxdeepin/dde-session-shell - #559
Open
deepin-ci-robot wants to merge 1 commit into
Open
deepin-ci-robot wants to merge 1 commit into
deepin-ci-robot wants to merge 1 commit into
Conversation
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#106
Contributor
Author
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-ci-robot 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 |
Reviewer's GuideSynchronizes security and robustness changes from dde-session-shell: authorizes suspend requests by Unix UID, removes shell-mediated process execution in several paths, hardens encryption key failures, and stops logging authentication tokens. Sequence diagram for authorized suspend requestssequenceDiagram
participant Caller
participant DBusLockAgent
participant DBus
participant SessionShell
Caller->>DBusLockAgent: Suspend(enable)
DBusLockAgent->>DBusLockAgent: getCallerBySender()
DBusLockAgent->>DBus: GetConnectionUnixUser(caller)
DBus-->>DBusLockAgent: callerUid
alt callerUid matches getuid()
DBusLockAgent->>SessionShell: process suspend request
else unauthorized or UID lookup fails
DBusLockAgent-->>Caller: reject request
end
Flow diagram for shell-free keyboard ungrabflowchart LR
A[ungrabKeyboard] --> B[QProcess start setxkbmap -query]
B --> C[Read current option]
C --> D[QProcess execute setxkbmap -option grab:break_actions]
D --> E[QProcess execute xdotool key XF86Ungrab]
E --> F[QProcess execute setxkbmap -option originmap]
Flow diagram for PAM autologin inhibitionflowchart LR
A[pam_sm_authenticate] --> B{Shell script exists?}
B -- no --> C[PAM_SUCCESS]
B -- yes --> D[fork]
D --> E[Child: execl shell script]
E --> F[Parent: waitpid]
F --> G{Exit status is zero?}
G -- yes --> H[PAM_PERM_DENIED]
G -- no --> I[PAM_SUCCESS]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/dde-lock/dbus/dbuslockagent.cpp" line_range="204" />
<code_context>
+ return true;
+ }
+
+ QString caller = msg.service();
+ if (caller.isEmpty()) {
+ return false;
</code_context>
<issue_to_address>
**Caller UID check uses destination**
When a caller running as a different user can send a method call to the lock service, `msg.service()` identifies the method-call destination, not its sender, so `GetConnectionUnixUser` checks the lock service’s UID and accepts a caller running as another user. That caller can invoke `Suspend(false)` and hide the lock screen when suspend locking is disabled.
Use the incoming message’s sender identity (for example, `msg.sender()`) for the UID lookup, and reject the call unless that sender’s UID matches the intended user.
</issue_to_address>| return true; | ||
| } | ||
|
|
||
| QString caller = msg.service(); |
There was a problem hiding this comment.
🟠 High · Caller UID check uses destination
When a caller running as a different user can send a method call to the lock service, msg.service() identifies the method-call destination, not its sender, so GetConnectionUnixUser checks the lock service’s UID and accepts a caller running as another user. That caller can invoke Suspend(false) and hide the lock screen when suspend locking is disabled.
Use the incoming message’s sender identity (for example, msg.sender()) for the UID lookup, and reject the call unless that sender’s UID matches the intended user.
Prompt for AI agents
In `src/dde-lock/dbus/dbuslockagent.cpp` at line 204:
**Caller UID check uses destination**
When a caller running as a different user can send a method call to the lock service, `msg.service()` identifies the method-call destination, not its sender, so `GetConnectionUnixUser` checks the lock service’s UID and accepts a caller running as another user. That caller can invoke `Suspend(false)` and hide the lock screen when suspend locking is disabled.
Use the incoming message’s sender identity (for example, `msg.sender()`) for the UID lookup, and reject the call unless that sender’s UID matches the intended user.
Contributor
Author
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： ['建议在 isCallerAuthorized() 函数前添加函数级注释，说明鉴权策略（同 UID 校验）和适用场景（会话总线）'] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： ['为 isCallerAuthorized() 添加函数级注释，例如：/* 检查 DBus 调用者 UID 是否与当前进程 UID 一致，仅允许同用户调用 */'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： [] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['建议后续考虑将 encrypt_helper.cpp 中的 srand(time(nullptr)) 替换为密码学安全的随机数生成器（如 RAND_bytes），提升对称密钥的随机性强度（此为既有问题，非本次引入）'] 💡 改进建议代码示例// 建议为 isCallerAuthorized() 添加函数级注释：
/**
* @brief 检查当前 DBus 调用者是否已授权
*
* 通过查询 DBus 守护进程获取调用者的 Unix UID，
* 与当前进程 UID 进行比较。仅允许同 UID 的调用者执行操作。
* 适用于会话总线（session bus）场景。
*
* @return true 表示调用者已授权或非方法调用消息；false 表示未授权
*/
bool DBusLockAgent::isCallerAuthorized()
{
QDBusMessage msg = message();
if (msg.type() != QDBusMessage::MethodCallMessage) {
return true;
}
QString caller = msg.service();
if (caller.isEmpty()) {
return false;
}
QDBusInterface dbusInterface("org.freedesktop.DBus", "/org/freedesktop/DBus",
"org.freedesktop.DBus", QDBusConnection::sessionBus());
QDBusReply<quint32> reply = dbusInterface.call("GetConnectionUnixUser", caller);
if (reply.isValid()) {
quint32 callerUid = reply.value();
if (callerUid != getuid()) {
qCWarning(DDE_SHELL) << "Caller UID mismatch:" << callerUid << "vs" << getuid();
return false;
}
return true;
}
qCWarning(DDE_SHELL) << "Failed to get caller UID:" << reply.error().message();
return false;
}本报告由 AI 代码审查工具自动生成 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Synchronize source files from linuxdeepin/dde-session-shell.
Source-pull-request: linuxdeepin/dde-session-shell#106
Summary by Sourcery
Harden session-shell security and process execution while removing sensitive authentication data from logs.
Bug Fixes:
Enhancements: