Skip to content

Commit aaf54c9

Browse files
ardelatoardelato
authored
Merge pull request #52 from iFixit/fix/privilege-escalation-c1-c2-m1
Fix: Priviledge Escalation
2 parents 8f04d6d + f41ee6a commit aaf54c9

5 files changed

Lines changed: 217 additions & 36 deletions

File tree

‎app/Console/Commands/UserCreate.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ public function handle(DiscourseService $discourseService): void
101101
'name' => $name,
102102
'email' => $email,
103103
'password' => Hash::make($password),
104-
'role' => $role,
105104
'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24),
106105
'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)),
107106
'calendar_hash' => Str::random(15),
@@ -118,6 +117,9 @@ public function handle(DiscourseService $discourseService): void
118117

119118
if ($user)
120119
{
120+
$user->role = $role;
121+
$user->save();
122+
121123
$this->info("User created {$user->name} (#{$user->id})");
122124
$this->info("Role: {$this->getRoleName($user->role)}");
123125

‎app/Http/Controllers/Auth/RegisterController.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use Illuminate\Routing\Controllers\HasMiddleware;
66
use Illuminate\Routing\Controllers\Middleware;
77
use App\Http\Controllers\Controller;
8+
use App\Models\Role;
89
use App\Models\User;
910
use Illuminate\Foundation\Auth\RegistersUsers;
1011
use Illuminate\Support\Facades\Hash;
@@ -62,11 +63,13 @@ protected function create(array $data): User
6263
'name' => $data['name'],
6364
'email' => $data['email'],
6465
'password' => Hash::make($data['password']),
65-
'role' => 4,
6666
'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24),
6767
'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)),
6868
]);
6969

70+
$user->role = Role::RESTARTER;
71+
$user->save();
72+
7073
Session::createSession($user->id);
7174

7275
return $user;

‎app/Http/Controllers/UserController.php‎

Lines changed: 27 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc
149149
return redirect()->back()->withErrors($validator)->withInput();
150150
}
151151

152-
if ($request->input('id') !== null) {
153-
$id = $request->input('id');
154-
} else {
155-
$id = Auth::id();
152+
$id = $request->input('id', Auth::id());
153+
if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) {
154+
abort(403);
156155
}
157156

158157
User::find($id)->update([
@@ -188,10 +187,9 @@ public function postProfileInfoEdit(Request $request, App\Helpers\Geocoder $geoc
188187

189188
public function postProfilePasswordEdit(Request $request): RedirectResponse
190189
{
191-
if ($request->input('id') !== null) {
192-
$id = $request->input('id');
193-
} else {
194-
$id = Auth::id();
190+
$id = $request->input('id', Auth::id());
191+
if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) {
192+
abort(403);
195193
}
196194

197195
$user = User::find($id);
@@ -346,10 +344,9 @@ public function postProfileTagsEdit(Request $request): RedirectResponse
346344

347345
public function postProfilePictureEdit(Request $request): RedirectResponse
348346
{
349-
if ($request->input('id') !== null) {
350-
$id = $request->input('id');
351-
} else {
352-
$id = Auth::id();
347+
$id = $request->input('id', Auth::id());
348+
if ($id != Auth::id() && !Auth::user()->hasRole('Administrator')) {
349+
abort(403);
353350
}
354351

355352
if (isset($_FILES) && ! empty($_FILES)) {
@@ -364,20 +361,19 @@ public function postProfilePictureEdit(Request $request): RedirectResponse
364361

365362
public function postAdminEdit(Request $request): RedirectResponse
366363
{
367-
if ($request->input('id') !== null) {
368-
$user_id = $request->input('id');
369-
} else {
370-
$user_id = Auth::id();
364+
if (!Auth::user()->hasRole('Administrator')) {
365+
abort(403);
371366
}
372367

368+
$user_id = $request->input('id', Auth::id());
369+
373370
$user = User::find($user_id);
374371

375372
$oldRole = $user->role;
376373

377374
// Set role for User
378-
$user->update([
379-
'role' => $request->input('user_role'),
380-
]);
375+
$user->role = $request->input('user_role');
376+
$user->save();
381377

382378
// If we are demoting from NetworkCoordinator, remove them from the list of coordinators for
383379
// any networks they are currently coordinating.
@@ -823,34 +819,30 @@ public function edit($id, Request $request)
823819
$Groups = new Group;
824820
$Groups = $Groups->findAll();
825821

826-
$data = $request->post();
827-
828822
if (! Fixometer::hasRole($User->find($id), 'Administrator')) {
829-
$sent_groups = $data['groups'];
823+
$sent_groups = $request->input('groups');
830824
}
831825

826+
$data = $request->only([
827+
'name', 'email', 'location', 'age', 'gender', 'country_code',
828+
'biography', 'language', 'newsletter', 'invites',
829+
]);
830+
832831
$error = false;
833832
// check for email in use
834833
$editingUser = $User->find($id);
835834
if ($editingUser->email !== $data['email'] && ! $User->checkEmail($data['email'])) {
836835
$error['email'] = 'The email you entered is already in use in our database. Please use another one.';
837836
}
838837

839-
if (! empty($data['new-password'])) {
840-
if ($data['new-password'] !== $data['password-confirm']) {
838+
if (! empty($request->input('new-password'))) {
839+
if ($request->input('new-password') !== $request->input('password-confirm')) {
841840
$error['password'] = 'The passwords are not identical!';
842841
} else {
843-
$data['password'] = Hash::make($data['new-password']);
842+
$data['password'] = Hash::make($request->input('new-password'));
844843
}
845844
}
846845

847-
unset($data['new-password']);
848-
unset($data['password-confirm']);
849-
850-
unset($data['groups']);
851-
unset($data['profile']);
852-
unset($data['id']);
853-
854846
if (! is_array($error)) {
855847
$u = $User->find($id)->update($data);
856848

@@ -1015,7 +1007,6 @@ public function postRegister(Request $request, $hash = null): RedirectResponse
10151007
'name' => $request->input('name'),
10161008
'email' => $request->input('email'),
10171009
'password' => Hash::make($request->input('password')),
1018-
'role' => $role,
10191010
'recovery' => substr(bin2hex(openssl_random_pseudo_bytes(32)), 0, 24),
10201011
'recovery_expires' => strftime('%Y-%m-%d %X', time() + (24 * 60 * 60)),
10211012
'country_code' => $request->input('country'),
@@ -1025,6 +1016,9 @@ public function postRegister(Request $request, $hash = null): RedirectResponse
10251016
'calendar_hash' => Str::random(15),
10261017
'username' => '',
10271018
]);
1019+
1020+
$user->role = $role;
1021+
$user->save();
10281022
}
10291023

10301024
$user->generateAndSetUsername();

‎app/Models/User.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,11 @@ class User extends Authenticatable implements Auditable, HasLocalePreference
4040
*
4141
* @var array
4242
*/
43+
// Note: `role` and `api_token` are intentionally excluded from $fillable.
44+
// They must be set via direct assignment ($user->role = X; $user->save())
45+
// to prevent privilege escalation via mass assignment (security: C2/M1).
4346
protected $fillable = [
44-
'name', 'email', 'password', 'role', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'api_token', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username',
47+
'name', 'email', 'password', 'recovery', 'recovery_expires', 'language', 'repair_network', 'location', 'age', 'gender', 'country_code', 'newsletter', 'invites', 'biography', 'consent_future_data', 'consent_past_data', 'consent_gdpr', 'number_of_logins', 'latitude', 'longitude', 'last_login_at', 'access_group_tag_id', 'calendar_hash', 'repairdir_role', 'mediawiki', 'username', 'external_user_id', 'external_username',
4548
];
4649

4750
/**

‎tests/Feature/Users/PrivilegeEscalationTest.php‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
<?php
2+
3+
namespace Tests\Feature;
4+
5+
use App\Models\Role;
6+
use App\Models\User;
7+
use Illuminate\Foundation\Testing\RefreshDatabase;
8+
use Tests\TestCase;
9+
10+
class PrivilegeEscalationTest extends TestCase
11+
{
12+
use RefreshDatabase;
13+
14+
/**
15+
* C1: Restarter cannot edit another user's profile info.
16+
*/
17+
public function test_restarter_cannot_edit_another_users_info(): void
18+
{
19+
$attacker = User::factory()->restarter()->create();
20+
$victim = User::factory()->restarter()->create();
21+
22+
$this->actingAs($attacker);
23+
24+
$response = $this->post('/profile/edit-info', [
25+
'id' => $victim->id,
26+
'email' => $victim->email,
27+
'name' => 'Hacked Name',
28+
'country' => $victim->country_code,
29+
'townCity' => $victim->location,
30+
'age' => $victim->age,
31+
'gender' => $victim->gender,
32+
'biography' => 'hacked',
33+
]);
34+
35+
$response->assertStatus(403);
36+
$this->assertNotEquals('Hacked Name', $victim->fresh()->name);
37+
}
38+
39+
/**
40+
* C1: Restarter cannot change another user's password.
41+
*/
42+
public function test_restarter_cannot_change_another_users_password(): void
43+
{
44+
$attacker = User::factory()->restarter()->create();
45+
$victim = User::factory()->restarter()->create();
46+
47+
$this->actingAs($attacker);
48+
49+
$response = $this->post('/profile/edit-password', [
50+
'id' => $victim->id,
51+
'current-password' => 'secret',
52+
'new-password' => 'hacked123',
53+
'new-password-repeat' => 'hacked123',
54+
]);
55+
56+
$response->assertStatus(403);
57+
}
58+
59+
/**
60+
* C1: Restarter cannot change another user's photo.
61+
*/
62+
public function test_restarter_cannot_change_another_users_photo(): void
63+
{
64+
$attacker = User::factory()->restarter()->create();
65+
$victim = User::factory()->restarter()->create();
66+
67+
$this->actingAs($attacker);
68+
69+
$response = $this->post('/profile/edit-photo', [
70+
'id' => $victim->id,
71+
]);
72+
73+
$response->assertStatus(403);
74+
}
75+
76+
/**
77+
* C1: Restarter cannot access admin edit settings.
78+
*/
79+
public function test_restarter_cannot_access_admin_edit_settings(): void
80+
{
81+
$attacker = User::factory()->restarter()->create();
82+
83+
$this->actingAs($attacker);
84+
85+
$response = $this->post('/profile/edit-admin-settings', [
86+
'id' => $attacker->id,
87+
'user_role' => Role::ROOT,
88+
]);
89+
90+
$response->assertStatus(403);
91+
$this->assertEquals(Role::RESTARTER, $attacker->fresh()->role);
92+
}
93+
94+
/**
95+
* C1 positive: Admin CAN change role via admin edit settings.
96+
*/
97+
public function test_admin_can_change_role_via_admin_edit(): void
98+
{
99+
$admin = User::factory()->administrator()->create();
100+
$user = User::factory()->restarter()->create();
101+
102+
$this->actingAs($admin);
103+
104+
$response = $this->post('/profile/edit-admin-settings', [
105+
'id' => $user->id,
106+
'user_role' => Role::HOST,
107+
'assigned_groups' => [],
108+
'preferences' => [],
109+
'permissions' => [],
110+
]);
111+
112+
$response->assertSessionHas('message');
113+
$this->assertEquals(Role::HOST, $user->fresh()->role);
114+
}
115+
116+
/**
117+
* C1 positive: Admin CAN edit another user's info.
118+
*/
119+
public function test_admin_can_edit_another_users_info(): void
120+
{
121+
$admin = User::factory()->administrator()->create();
122+
$user = User::factory()->restarter()->create();
123+
124+
$this->actingAs($admin);
125+
126+
$response = $this->post('/profile/edit-info', [
127+
'id' => $user->id,
128+
'email' => $user->email,
129+
'name' => 'Updated By Admin',
130+
'country' => $user->country_code,
131+
'townCity' => $user->location,
132+
'age' => $user->age,
133+
'gender' => $user->gender,
134+
'biography' => $user->biography,
135+
]);
136+
137+
$response->assertRedirect();
138+
$this->assertEquals('Updated By Admin', $user->fresh()->name);
139+
}
140+
141+
/**
142+
* C2/M1: User cannot escalate role via /user/edit endpoint.
143+
*/
144+
public function test_user_cannot_escalate_role_via_edit_endpoint(): void
145+
{
146+
$admin = User::factory()->administrator()->create();
147+
$user = User::factory()->restarter()->create();
148+
149+
$this->actingAs($admin);
150+
151+
$response = $this->post("/user/edit/{$user->id}", [
152+
'name' => $user->name,
153+
'email' => $user->email,
154+
'role' => Role::ROOT,
155+
]);
156+
157+
$this->assertNotEquals(Role::ROOT, $user->fresh()->role);
158+
}
159+
160+
/**
161+
* C2/M1: User cannot overwrite api_token via /user/edit endpoint.
162+
*/
163+
public function test_user_cannot_overwrite_api_token_via_edit_endpoint(): void
164+
{
165+
$admin = User::factory()->administrator()->create();
166+
$user = User::factory()->restarter()->create();
167+
$originalToken = $user->api_token;
168+
169+
$this->actingAs($admin);
170+
171+
$response = $this->post("/user/edit/{$user->id}", [
172+
'name' => $user->name,
173+
'email' => $user->email,
174+
'api_token' => 'stolen_token_value',
175+
]);
176+
177+
$this->assertEquals($originalToken, $user->fresh()->api_token);
178+
}
179+
}

0 commit comments

Comments
 (0)