# Patch Analysis — GHSA-h9qx-v5xp-ph8p (EGroupware)

## Overview

The fix for GHSA-h9qx-v5xp-ph8p was released in EGroupware tags
`26.2.20260224` and `23.1.20260224` (CVE-2026-27823). It addresses three
distinct primitives described in the advisory:
1. Authorization bypass in `SmallPartMediaRecorder::ajax_upload()`
2. Arbitrary file write via path traversal in `Bo::videoPath()`
3. Arbitrary file read via path traversal in `importexport_export_ui::download()`

## What the fix changes

### 1. `smallpart/src/Widgets/SmallPartMediaRecorder.php`

**Before (vulnerable, 26.2.20260216):**
```php
if (!$bo->isTeacher($data['video']['course_id']))          // attacker-controlled array
    throw new Api\Exception\NoPermission();
...
if ($data['video']['video_hash']) {
    $filePath = $bo->videoPath($data['video'], true);      // attacker-controlled path
```

**After (fixed, 26.2.20260224):**
```php
if (!$bo->isTeacher((int)$data['video']['course_id']) ||   // (int) cast defeats array bypass
    !($video = $bo->readVideo((int)$data['video']['video_id'])))  // requires real DB video
    throw new Api\Exception\NoPermission();
...
if ($video['video_hash']) {
    $filePath = $bo->videoPath($video, true);              // DB-sourced $video
```

**Changes:**
- `course_id` is cast to `(int)` before `isTeacher()` — a PHP array casts to
  `1`, and `isTeacher(1)` reads real ACL from the DB, defeating the
  `participants`-array bypass.
- `readVideo((int)$data['video']['video_id'])` requires a real DB video row;
  if none exists, `NoPermission` is thrown.
- `videoPath()` receives the DB-sourced `$video` record, so `video_hash` and
  `video_type` are no longer attacker-controlled.

### 2. `smallpart/src/Bo.php` — `videoPath()` method

**Added (lines 821-823):**
```php
// stop possible pass-traversal
if (basename($video['video_hash']) !== $video['video_hash']) throw new Api\Exception\WrongParameter("Invalid video_hash!");
if (basename($video['video_type']) !== $video['video_type'])  throw new Api\Exception\WrongParameter("Invalid video_type!");
```

**Effect:** Even if traversal characters (`../`) reach `videoPath()` through
any caller, the `basename()` check rejects them. This is defense-in-depth that
protects ALL callers of `videoPath()`, not just `ajax_upload`.

### 3. `importexport/inc/class.importexport_export_ui.inc.php` (egroupware core)

**Added in `download()` method:**
```php
// guard against path traversal
if (basename($tmpfname) !== $tmpfname) die();
```

Plus `basename()` wrapping of `$_GET['_appname']`, `$_GET['filename']`,
`$_GET['_suffix']`, and `$_GET['_type']`.

**Effect:** Prevents the arbitrary-file-read primitive via the `_filename`
parameter (e.g., reading `header.inc.php`).

## Files changed by the fix

### smallpart repo (2 files)
- `src/Widgets/SmallPartMediaRecorder.php` — 10 lines changed
- `src/Bo.php` — 5 lines added (basename guards in `videoPath()`)

### egroupware core repo (183 files, security-relevant subset)
- `importexport/inc/class.importexport_export_ui.inc.php` — 19 lines changed
  (basename guards in `download()`)
- Other files are language/template updates and unrelated bug fixes.

## What assumptions the fix makes

1. **Only `ajax_upload` reaches `isTeacher()` with attacker-controlled data:**
   The fix assumes that casting `course_id` to `(int)` in `ajax_upload()` is
   sufficient because no other caller passes request-controlled array data to
   `isTeacher()`/`isParticipant()`. This assumption is **FALSE** — multiple
   other callers pass raw request data.

2. **`videoPath()` traversal is the only file-write sink:** The `basename()`
   guards in `videoPath()` protect all file-write paths in the smallpart app.
   This assumption appears **correct** — all file writes in smallpart go
   through `videoPath()`.

3. **`_filename` traversal is the only file-read sink:** The `basename()` guard
   in `importexport_export_ui::download()` addresses the read primitive. This
   appears correct for the specific advisory.

## What code paths the fix does NOT cover

### `Bo::isParticipant()` — the root cause (NOT FIXED)

The `isParticipant()` function (Bo.php ~line 2339) still contains:

```php
if (is_array($course) && isset($course['participants']))
{
    $participants = array_filter($course['participants'], ...);
    $this->course_acl[$course['course_id']] = $participants ? current($participants)['participant_role'] : null;
}
```

This code caches the attacker-supplied `participant_role` and uses it for
authorization decisions. The fix did NOT modify this function. It is identical
across `26.2.20260216` → `26.2.20260224` → `26.7.20260702`.

### Unfixed callers of `isTeacher()` / `isParticipant()` / `isAdmin()`

| Caller | File | Receives request data? | Cast to (int)? |
|--------|------|----------------------|----------------|
| `Overlay::aclCheck` | `Overlay.php:816` | Yes (`$data['course_id']` from AJAX) | **No** |
| `Student\Ui::ajax_listComments` | `Student/Ui.php:988` | Yes (`$where` from AJAX) | **No** |
| `Student\Ui::showCommentButton` | `Student/Ui.php:1315` | Yes (`$content` from etemplate) | **No** |
| `Bo::save` | `Bo.php:2872` | Yes (`$keys['course_id']` from etemplate) | **No** |
| `Bo::close` | `Bo.php:2698` | Yes (`$id` from AJAX via `Courses::ajax_action`) | **No** |
| `Bo::addLivefeedback` | `Bo.php:947` | Yes (`$course` from etemplate) | **No** |
| `Bo::deleteVideo` | `Bo.php:1246` | Yes (`$video['course_id']` from etemplate) | **No** |

All of these callers pass data that could be an attacker-controlled array with
a `participants` key, bypassing the authorization check.

### Downstream guards that partially mitigate (pre-existing, NOT part of the fix)

- `Overlay::write()` — `is_int($data['course_id']) || is_numeric(...)` (present
  since before the fix; blocks arrays)
- `Overlay::delete()` — `is_int($what['course_id'])` (present since before the
  fix; blocks arrays)
- `Bo::listComments()` — has its own ACL via `read()` (DB-sourced; blocks
  non-participants)

These guards prevent the **specific** write/delete operations from completing
when `course_id` is an array, but they do NOT prevent the authz bypass itself.
Other callers without such guards (e.g., `Bo::save()`) are fully exposed.

## Comparison of behavior before and after the fix

| Scenario | Vulnerable (26.2.20260216) | Fixed (26.2.20260224) |
|----------|---------------------------|----------------------|
| `ajax_upload` with array `course_id` (RCE chain) | ✅ RCE achieved | ❌ Blocked by `(int)` cast + `readVideo()` |
| `ajax_upload` with traversal in `video_hash` | ✅ File write | ❌ Blocked by `basename()` + DB-sourced video |
| `importexport download` with traversal in `_filename` | ✅ File read | ❌ Blocked by `basename()` |
| `Overlay::ajax_write` with array `course_id` (authz bypass) | ✅ Authz bypassed (reaches `write()`, blocked by type guard) | ✅ **Authz STILL bypassed** (same behavior) |
| `Bo::save()` with array `course_id` (course modification) | ✅ Authz bypassed | ✅ **Authz STILL bypassed** (not tested runtime, code analysis) |

## Completeness assessment

The fix is **incomplete**:
- ✅ The specific RCE chain (authz bypass → file write → RCE) is properly
  closed by the `(int)` cast + `readVideo()` + `basename()` guards.
- ✅ The file-read primitive is properly closed by `basename()` in
  `importexport_export_ui::download()`.
- ❌ The **root cause** (`isParticipant()` trusting request-supplied
  `participants` arrays) is NOT fixed.
- ❌ Multiple other callers of `isTeacher()`/`isParticipant()`/`isAdmin()`
  remain vulnerable to the same authz bypass on the fixed version.
- ❌ The latest version (`26.7.20260702`) also does not fix `isParticipant()`.

## Target threat model

The smallpart README states "Server-side ACL enforcement for all user
actions" as a feature. The `isParticipant()` bypass violates this stated
threat model — a low-privileged authenticated user can assert any role
(teacher, admin) for any course by supplying a crafted `participants` array.
No `SECURITY.md` file exists in the smallpart or egroupware repositories.
The advisory itself describes the authz bypass as a critical vulnerability,
so this is clearly within the target's security scope.