Skip to content

Commit 80ad47e

Browse files
CodeDoctorDECodeDoctorDE
committed
Improve app and server security checks
1 parent e119b52 commit 80ad47e

7 files changed

Lines changed: 147 additions & 35 deletions

File tree

‎api/lib/src/event/process/client.dart‎

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,28 @@ import 'package:dart_leap/dart_leap.dart';
44
import 'package:networker/networker.dart';
55
import 'package:setonix_api/setonix_api.dart';
66

7+
const _maxTextLength = 2000;
8+
const _maxIdentifierLength = 256;
9+
const _maxImagesPerRequest = 100;
10+
const _maxPacksPerRequest = 128;
11+
const _maxBoardsPerRequest = 256;
12+
13+
bool _isReasonableIdentifier(String value) =>
14+
value.isNotEmpty && value.length <= _maxIdentifierLength;
15+
716
bool isValidClientEvent(
817
WorldEvent event,
918
Channel channel,
1019
WorldState state, {
1120
required AssetManager assetManager,
1221
ChallengeManager? challengeManager,
1322
}) => switch (event) {
14-
TeamJoinRequest() => state.info.teams.containsKey(event.team),
15-
TeamLeaveRequest() => state.info.teams.containsKey(event.team),
23+
TeamJoinRequest() =>
24+
_isReasonableIdentifier(event.team) &&
25+
state.info.teams.containsKey(event.team),
26+
TeamLeaveRequest() =>
27+
_isReasonableIdentifier(event.team) &&
28+
state.info.teams.containsKey(event.team),
1629
CellRollRequest() =>
1730
event.object?.inRange(
1831
0,
@@ -68,9 +81,19 @@ bool isValidClientEvent(
6881
1,
6982
),
7083
TeamRemoved() => state.info.teams.containsKey(event.team),
71-
PacksChangeRequest() => channel == kAuthorityChannel,
84+
PacksChangeRequest() =>
85+
channel == kAuthorityChannel &&
86+
event.packs.length <= _maxPacksPerRequest &&
87+
event.packs.every(_isReasonableIdentifier),
88+
MessageRequest() =>
89+
event.message.isNotEmpty && event.message.length <= _maxTextLength,
90+
BoardsSpawnRequest() =>
91+
_isReasonableIdentifier(event.table) &&
92+
event.assets.length <= _maxBoardsPerRequest &&
93+
event.assets.values.expand((e) => e).length <= _maxBoardsPerRequest,
7294
BoardMoveRequest() =>
73-
event.from != event.to &&
95+
_isReasonableIdentifier(event.table) &&
96+
event.from != event.to &&
7497
event.index.inRange(
7598
0,
7699
state
@@ -81,6 +104,11 @@ bool isValidClientEvent(
81104
1,
82105
),
83106
ModeChangeRequest() => channel == kAuthorityChannel,
107+
DialogCloseRequest() => _isReasonableIdentifier(event.id),
108+
ImagesRequest() =>
109+
event.ids.length <= _maxImagesPerRequest &&
110+
event.ids.every(_isReasonableIdentifier),
111+
ToolbarActionRequest() => _isReasonableIdentifier(event.actionId),
84112
_ => true,
85113
};
86114

@@ -228,7 +256,7 @@ Future<ServerResponse?> processClientEvent(
228256
case CellRollRequest():
229257
final table = state.getTableOrDefault(event.cell.table);
230258
var cell = table.getCell(event.cell.position);
231-
final random = Random();
259+
final random = Random.secure();
232260
GameObject roll(GameObject object) {
233261
final figure = assetManager.getFigure(object.asset);
234262
if (figure == null || !figure.rollable) return object;
@@ -255,7 +283,7 @@ Future<ServerResponse?> processClientEvent(
255283
final cell = table.cells[event.cell.position];
256284
if (cell == null) return null;
257285
final positions = List<int>.generate(cell.objects.length, (i) => i)
258-
..shuffle();
286+
..shuffle(Random.secure());
259287
return UpdateServerResponse.builder(
260288
CellShuffled(event.cell, positions),
261289
kAnyChannel,
@@ -393,11 +421,13 @@ Future<ServerResponse?> processClientEvent(
393421
if (challengeManager == null) return null;
394422
final verified = await event.verify(challenge);
395423
if (!verified) {
424+
final newChallenge = challengeManager.generateNewChallenge(channel);
396425
return UpdateServerResponse.builder(
397-
AuthenticatedRequested(challenge, isRequired: true),
426+
AuthenticatedRequested(newChallenge, isRequired: true),
398427
channel,
399428
);
400429
}
430+
challengeManager.removeChallenge(channel);
401431
SetonixUser? user;
402432
try {
403433
user = await userManager?.addUser(

‎app/lib/api/open.dart‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,26 @@ Future<SetonixFile?> openFile() async {
7676
if (file == null) return null;
7777
final bytes = file.bytes;
7878
if (bytes == null) return null;
79-
final data = SetonixFile(bytes, file.name);
79+
final data = SetonixFile(bytes);
8080
return data;
8181
}
8282

83+
String _sanitizeImportedName(String name, String fallback) {
84+
var sanitized = name.trim();
85+
if (sanitized.isEmpty) {
86+
sanitized = fallback.trim();
87+
}
88+
sanitized = sanitized.split('/').last.split(r'\').last.trim();
89+
const suffix = '.stnx';
90+
if (sanitized.toLowerCase().endsWith(suffix)) {
91+
sanitized = sanitized.substring(0, sanitized.length - suffix.length);
92+
}
93+
if (sanitized.isEmpty || sanitized == '.' || sanitized == '..') {
94+
return DateTime.now().millisecondsSinceEpoch.toString();
95+
}
96+
return sanitized;
97+
}
98+
8399
Future<void> importFile(
84100
BuildContext context,
85101
SetonixFileSystem fileSystem,
@@ -138,9 +154,15 @@ Future<void> importFileData(
138154
case SetonixFileType.pack:
139155
await fileSystem.packSystem.updateFile(id, file);
140156
case SetonixFileType.template:
141-
await fileSystem.templateSystem.createFile(metadata.name, data);
157+
await fileSystem.templateSystem.createFile(
158+
_sanitizeImportedName(metadata.name, file.identifier),
159+
data,
160+
);
142161
case SetonixFileType.game:
143-
await fileSystem.worldSystem.createFile(metadata.name, data);
162+
await fileSystem.worldSystem.createFile(
163+
_sanitizeImportedName(metadata.name, file.identifier),
164+
data,
165+
);
144166
case SetonixFileType.accounts:
145167
await fileSystem.importAccountsFromData(data);
146168
}

‎app/lib/services/file_system.dart‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ enum PackDownloadResult {
2020
bool get isSuccess => this == success || this == alreadyExists;
2121
}
2222

23+
bool _isSafeDownloadUri(Uri uri) {
24+
if (uri.isScheme('https')) return true;
25+
if (!uri.isScheme('http')) return false;
26+
return uri.host == 'localhost' ||
27+
uri.host == '127.0.0.1' ||
28+
uri.host == '::1';
29+
}
30+
2331
class SetonixFileSystem {
2432
SetonixFile? _corePack;
2533
final TypedKeyFileSystem<SetonixFile> packSystem;
@@ -180,9 +188,7 @@ class SetonixFileSystem {
180188
try {
181189
final uri = Uri.tryParse(url);
182190
if (uri == null) return PackDownloadResult.invalidUri;
183-
if (!uri.isScheme('http') && !uri.isScheme('https')) {
184-
return PackDownloadResult.invalidUri;
185-
}
191+
if (!_isSafeDownloadUri(uri)) return PackDownloadResult.invalidUri;
186192
if (!force && await packSystem.hasKey(expectedIdentifier)) {
187193
return PackDownloadResult.alreadyExists;
188194
}

‎metadata/en-US/changelogs/13.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
* Fix loading the Blackjack game mode
33
* Prevent creating games without a name
44
* Fix playing a game without a selection showing a missing route
5+
* Improve app and server security checks
6+
* Harden multiplayer authentication and imported file names
57
* Improve Luau script error messages with game mode and script context
68

79
Read more here: https://linwood.dev/setonix/0.8.0

‎server/lib/src/bloc.dart‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@ Future<ServerProcessed> _computeEvent(
1919

2020
const scriptSuffix = '.lua';
2121

22+
String _sanitizeWorldFileName(String worldName) {
23+
final name = p.basename(worldName);
24+
if (name.isEmpty || name == '.' || name == '..') {
25+
return defaultWorldName;
26+
}
27+
return name;
28+
}
29+
2230
class WorldBloc extends Bloc<PlayableWorldEvent, WorldState>
2331
with ServerInterface {
2432
final SetonixServer server;
@@ -156,8 +164,13 @@ class WorldBloc extends Bloc<PlayableWorldEvent, WorldState>
156164
}
157165

158166
Future<void> save({bool force = false}) async {
167+
final safeWorldName = _sanitizeWorldFileName(worldName);
159168
var file = File(
160-
'${worldName == defaultWorldName ? defaultWorldName : '${SetonixServer.worldDirectory}/$worldName'}${SetonixServer.worldSuffix}',
169+
p.join(
170+
server.rootDirectory,
171+
SetonixServer.worldDirectory,
172+
'$safeWorldName${SetonixServer.worldSuffix}',
173+
),
161174
);
162175
if (!await file.exists()) {
163176
await file.create(recursive: true);

‎server/lib/src/server.dart‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@ String limitOutput(Object? value, [int limit = 500]) {
3131
return string;
3232
}
3333

34+
bool _isSafeRemoteEndpoint(Uri uri) {
35+
if (uri.isScheme('https')) return true;
36+
if (!uri.isScheme('http')) return false;
37+
return uri.host == 'localhost' ||
38+
uri.host == '127.0.0.1' ||
39+
uri.host == '::1';
40+
}
41+
3442
final class SetonixServer {
3543
static const String worldDirectory = 'worlds';
3644
static const String worldSuffix = '.stnx';
@@ -123,7 +131,18 @@ final class SetonixServer {
123131
final apiEndpoint = configManager.apiEndpoint;
124132
UserService userService;
125133
if (apiEndpoint.isNotEmpty) {
126-
userService = RemoteUserService(apiEndpoint: apiEndpoint);
134+
final uri = Uri.tryParse(apiEndpoint);
135+
if (uri == null || !_isSafeRemoteEndpoint(uri)) {
136+
throw ArgumentError.value(
137+
apiEndpoint,
138+
'apiEndpoint',
139+
'Remote user API endpoints must use HTTPS, except localhost HTTP.',
140+
);
141+
}
142+
userService = RemoteUserService(
143+
apiEndpoint: apiEndpoint,
144+
endpointSecret: configManager.endpointSecret,
145+
);
127146
} else {
128147
final fileService = FileUserService();
129148
await fileService.setup(rootPath: rootDirectory);
@@ -200,8 +219,9 @@ final class SetonixServer {
200219
level: LogLevel.warning,
201220
);
202221
}
222+
final bindAddress = await _resolveBindAddress(configManager.host);
203223
final server = _server = NetworkerSocketServer(
204-
InternetAddress.anyIPv4,
224+
bindAddress,
205225
configManager.port,
206226
securityContext: securityContext,
207227
filterConnections: buildFilterConnections(
@@ -244,6 +264,13 @@ final class SetonixServer {
244264
await loadWorlds();
245265
}
246266

267+
static Future<InternetAddress> _resolveBindAddress(String host) async {
268+
final parsed = InternetAddress.tryParse(host);
269+
if (parsed != null) return parsed;
270+
final addresses = await InternetAddress.lookup(host);
271+
return addresses.first;
272+
}
273+
247274
void _onClientEvent(
248275
NetworkerPacket<WorldEvent> packet, {
249276
bool force = false,

‎server/lib/src/services/user/remote.dart‎

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,26 @@ import 'package:http/http.dart' as http;
55
import 'package:setonix_server/setonix_server.dart';
66

77
final class RemoteUserService extends UserService {
8+
static const requestTimeout = Duration(seconds: 10);
9+
810
final String apiEndpoint;
911
final String? endpointSecret;
1012

1113
RemoteUserService({required this.apiEndpoint, this.endpointSecret});
1214

1315
Map<String, String> get headers =>
14-
endpointSecret != null ? {'Authorization': 'Bearer $endpointSecret'} : {};
16+
endpointSecret != null && endpointSecret!.isNotEmpty
17+
? {'Authorization': 'Bearer $endpointSecret'}
18+
: {};
1519

1620
@override
1721
Future<SetonixUser?> getUser(String fingerprint) async {
18-
final response = await http.get(
19-
Uri.parse('$apiEndpoint/user/${Uri.encodeComponent(fingerprint)}'),
20-
headers: headers,
21-
);
22+
final response = await http
23+
.get(
24+
Uri.parse('$apiEndpoint/user/${Uri.encodeComponent(fingerprint)}'),
25+
headers: headers,
26+
)
27+
.timeout(requestTimeout);
2228
if (response.statusCode >= 200 && response.statusCode < 300) {
2329
if (response.body.isEmpty) return null; // No user found
2430
return SetonixUserMapper.fromJson(response.body);
@@ -28,10 +34,12 @@ final class RemoteUserService extends UserService {
2834

2935
@override
3036
Future<SetonixUser?> getUserFromName(String name) async {
31-
final response = await http.get(
32-
Uri.parse('$apiEndpoint/user?name=${Uri.encodeComponent(name)}'),
33-
headers: headers,
34-
);
37+
final response = await http
38+
.get(
39+
Uri.parse('$apiEndpoint/user?name=${Uri.encodeComponent(name)}'),
40+
headers: headers,
41+
)
42+
.timeout(requestTimeout);
3543
if (response.statusCode == 200) {
3644
return SetonixUserMapper.fromJson(response.body);
3745
}
@@ -51,11 +59,13 @@ final class RemoteUserService extends UserService {
5159
'onWhitelist': onWhitelist,
5260
'lastLogin': lastLogin?.millisecondsSinceEpoch,
5361
});
54-
final response = await http.patch(
55-
Uri.parse('$apiEndpoint/user/${Uri.encodeComponent(fingerprint)}'),
56-
headers: {...headers, 'Content-Type': 'application/json'},
57-
body: body,
58-
);
62+
final response = await http
63+
.patch(
64+
Uri.parse('$apiEndpoint/user/${Uri.encodeComponent(fingerprint)}'),
65+
headers: {...headers, 'Content-Type': 'application/json'},
66+
body: body,
67+
)
68+
.timeout(requestTimeout);
5969

6070
if (response.statusCode == 200) {
6171
return true;
@@ -67,11 +77,13 @@ final class RemoteUserService extends UserService {
6777
'onWhitelist': onWhitelist,
6878
'lastLogin': lastLogin?.millisecondsSinceEpoch,
6979
});
70-
final createResponse = await http.post(
71-
Uri.parse('$apiEndpoint/user'),
72-
headers: {...headers, 'Content-Type': 'application/json'},
73-
body: createBody,
74-
);
80+
final createResponse = await http
81+
.post(
82+
Uri.parse('$apiEndpoint/user'),
83+
headers: {...headers, 'Content-Type': 'application/json'},
84+
body: createBody,
85+
)
86+
.timeout(requestTimeout);
7587
return createResponse.statusCode >= 200 &&
7688
createResponse.statusCode < 300;
7789
}

0 commit comments

Comments
 (0)