mirror of https://github.com/openclaw/openclaw.git
fix(matrix): correct DM classification without trusting remote user's is_direct flag
Problem: Matrix DM classification logic had security vulnerabilities: 1. Unreachable code branch when is_direct flag was absent 2. When is_direct: true, skipped 2-member check (shared rooms misclassified as DMs) 3. **CWE-285: Improper Authorization** - trusted remote user's is_direct flag Security Issues: - Remote attacker could set is_direct=true on their membership to force DM classification - Remote attacker could set is_direct=false to bypass DM-only restrictions - Both could lead to policy bypass (DM allowlist/pairing checks) Fix: - hasDirectMatrixMemberFlag() returns boolean | null for local user only - isStrictDirectMembership() only trusts local user's is_direct (selfUserId) - Removed directViaSender lookups entirely (do not trust remote-controlled data) - Falls back to strict 2-member check when is_direct is false/null Key Insights: - In Matrix, m.room.member.content.is_direct is set by each member themselves - Only trust signals the bot controls (local user's membership state) - 2-member check remains as safe fallback that cannot be manipulated Closes #56599
This commit is contained in:
parent
dd9d0bdd8e
commit
19a3a30dc0
|
|
@ -21,10 +21,28 @@ export function isStrictDirectMembership(params: {
|
|||
selfUserId?: string | null;
|
||||
remoteUserId?: string | null;
|
||||
joinedMembers?: readonly string[] | null;
|
||||
isDirectFlag?: boolean | null;
|
||||
}): boolean {
|
||||
const selfUserId = trimMaybeString(params.selfUserId);
|
||||
const remoteUserId = trimMaybeString(params.remoteUserId);
|
||||
const joinedMembers = params.joinedMembers ?? [];
|
||||
|
||||
// Only trust is_direct from local user's membership state (selfUserId).
|
||||
// Remote user's is_direct is NOT trusted (CWE-285: authorization bypass).
|
||||
|
||||
// When local user's is_direct=true, verify it's a true 2-person DM
|
||||
if (params.isDirectFlag === true) {
|
||||
return Boolean(
|
||||
selfUserId &&
|
||||
remoteUserId &&
|
||||
joinedMembers.length === 2 &&
|
||||
joinedMembers.includes(selfUserId) &&
|
||||
joinedMembers.includes(remoteUserId),
|
||||
);
|
||||
}
|
||||
|
||||
// When is_direct=false or null, fall back to strict 2-member check
|
||||
// This prevents attackers from forcing non-DM classification with is_direct=false
|
||||
return Boolean(
|
||||
selfUserId &&
|
||||
remoteUserId &&
|
||||
|
|
@ -49,16 +67,25 @@ export async function hasDirectMatrixMemberFlag(
|
|||
client: MatrixClient,
|
||||
roomId: string,
|
||||
userId?: string | null,
|
||||
): Promise<boolean> {
|
||||
): Promise<boolean | null> {
|
||||
const normalizedUserId = trimMaybeString(userId);
|
||||
if (!normalizedUserId) {
|
||||
return false;
|
||||
return null;
|
||||
}
|
||||
try {
|
||||
const state = await client.getRoomStateEvent(roomId, "m.room.member", normalizedUserId);
|
||||
return state?.is_direct === true;
|
||||
// Return true if is_direct is explicitly true, false if explicitly false, null if absent
|
||||
if (state?.is_direct === true) {
|
||||
return true;
|
||||
}
|
||||
if (state?.is_direct === false) {
|
||||
return false;
|
||||
}
|
||||
// is_direct field is absent from the membership event
|
||||
return null;
|
||||
} catch {
|
||||
return false;
|
||||
// API/network error - treat as unavailable
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -95,8 +122,9 @@ export async function inspectMatrixDirectRoomEvidence(params: {
|
|||
joinedMembers,
|
||||
strict,
|
||||
viaMemberState:
|
||||
(await hasDirectMatrixMemberFlag(params.client, params.roomId, params.remoteUserId)) ||
|
||||
(await hasDirectMatrixMemberFlag(params.client, params.roomId, selfUserId)),
|
||||
(await hasDirectMatrixMemberFlag(params.client, params.roomId, params.remoteUserId)) ===
|
||||
true ||
|
||||
(await hasDirectMatrixMemberFlag(params.client, params.roomId, selfUserId)) === true,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -82,10 +82,10 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr
|
|||
const resolveDirectMemberFlag = async (
|
||||
roomId: string,
|
||||
userId?: string | null,
|
||||
): Promise<boolean> => {
|
||||
): Promise<boolean | null> => {
|
||||
const normalizedUserId = userId?.trim();
|
||||
if (!normalizedUserId) {
|
||||
return false;
|
||||
return null;
|
||||
}
|
||||
const cacheKey = `${roomId}\n${normalizedUserId}`;
|
||||
const cached = directMemberFlagCache.get(cacheKey);
|
||||
|
|
@ -113,10 +113,19 @@ export function createDirectRoomTracker(client: MatrixClient, opts: DirectRoomTr
|
|||
const { roomId, senderId } = params;
|
||||
const selfUserId = params.selfUserId ?? (await ensureSelfUserId());
|
||||
const joinedMembers = await resolveJoinedMembers(roomId);
|
||||
|
||||
// Check is_direct flag from local user's membership state only.
|
||||
// Do NOT trust remote sender's is_direct (CWE-285: improper authorization).
|
||||
// In Matrix, m.room.member.content.is_direct is set by each member themselves,
|
||||
// so a malicious remote user could manipulate it to bypass DM/room policies.
|
||||
const directViaSelf = await resolveDirectMemberFlag(roomId, selfUserId);
|
||||
const isDirectFlag: boolean | null = directViaSelf;
|
||||
|
||||
const strictDirectMembership = isStrictDirectMembership({
|
||||
selfUserId,
|
||||
remoteUserId: senderId,
|
||||
joinedMembers,
|
||||
isDirectFlag,
|
||||
});
|
||||
|
||||
try {
|
||||
|
|
|
|||
Loading…
Reference in New Issue