Skip to content

Commit c63ab6d

Browse files
leif-scalityleif-scality
committed
CLDSRV-744: add missing md5 checksum checks
1 parent 1ca5ec8 commit c63ab6d

13 files changed

Lines changed: 468 additions & 214 deletions

‎lib/api/api.js‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ const parseCopySource = require('./apiUtils/object/parseCopySource');
7474
const { tagConditionKeyAuth } = require('./apiUtils/authorization/tagConditionKeys');
7575
const { isRequesterASessionUser } = require('./apiUtils/authorization/permissionChecks');
7676
const checkHttpHeadersSize = require('./apiUtils/object/checkHttpHeadersSize');
77+
const { validateMethodChecksumNoChunking } = require('./apiUtils/integrity/validateChecksums');
7778

7879
const monitoringMap = policies.actionMaps.actionMonitoringMapS3;
7980

@@ -242,8 +243,16 @@ const api = {
242243
{ postLength });
243244
return next(errors.InvalidRequest);
244245
}
246+
247+
const buff = Buffer.concat(post, postLength);
248+
249+
const err = validateMethodChecksumNoChunking(request, buff, log);
250+
if (err) {
251+
return next(err);
252+
}
253+
245254
// Convert array of post buffers into one string
246-
request.post = Buffer.concat(post, postLength).toString();
255+
request.post = buff.toString();
247256
return next(null, userInfo, authorizationResults, streamingV4Params, infos);
248257
});
249258
return undefined;

‎lib/api/apiUtils/integrity/validateChecksums.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
const crypto = require('crypto');
2+
const { errors: ArsenalErrors } = require('arsenal');
3+
const { config } = require('../../../Config');
4+
5+
const ChecksumError = Object.freeze({
6+
MD5Mismatch: 'MD5Mismatch',
7+
MissingChecksum: 'MissingChecksum',
8+
});
9+
10+
/**
11+
* validateChecksumsNoChunking - Validate the checksums of a request.
12+
* @param {object} headers - http headers
13+
* @param {Buffer} body - http request body
14+
* @return {object} - error
15+
*/
16+
function validateChecksumsNoChunking(headers, body) {
17+
if (headers && 'content-md5' in headers) {
18+
const md5 = crypto.createHash('md5').update(body).digest('base64');
19+
if (md5 !== headers['content-md5']) {
20+
return { error: ChecksumError.MD5Mismatch, details: { calculated: md5, expected: headers['content-md5'] } };
21+
}
22+
23+
return null;
24+
}
25+
26+
return { error: ChecksumError.MissingChecksum, details: null };
27+
}
28+
29+
function defaultValidationFunc(request, body, log) {
30+
const err = validateChecksumsNoChunking(request.headers, body);
31+
if (err && err.error !== ChecksumError.MissingChecksum) {
32+
log.debug('failed checksum validation', { method: request.apiMethod }, err);
33+
return ArsenalErrors.BadDigest;
34+
}
35+
36+
return null;
37+
}
38+
39+
const methodValidationFunc = Object.freeze({
40+
'bucketPutACL': defaultValidationFunc,
41+
'bucketPutCors': defaultValidationFunc,
42+
'bucketPutEncryption': defaultValidationFunc,
43+
'bucketPutLifecycle': defaultValidationFunc,
44+
'bucketPutNotification': defaultValidationFunc,
45+
'bucketPutObjectLock': defaultValidationFunc,
46+
'bucketPutPolicy': defaultValidationFunc,
47+
'bucketPutReplication': defaultValidationFunc,
48+
'bucketPutVersioning': defaultValidationFunc,
49+
'bucketPutWebsite': defaultValidationFunc,
50+
// TODO: DeleteObjects requires a checksum. Should return an error if ChecksumError.MissingChecksum.
51+
'multiObjectDelete': defaultValidationFunc,
52+
'objectPutACL': defaultValidationFunc,
53+
'objectPutLegalHold': defaultValidationFunc,
54+
'objectPutTagging': defaultValidationFunc,
55+
'objectPutRetention': defaultValidationFunc,
56+
});
57+
58+
/**
59+
* validateMethodChecksumsNoChunking - Validate the checksums of a request.
60+
* @param {object} request - http request
61+
* @param {Buffer} body - http request body
62+
* @param {object} log - logger
63+
* @return {object} - error
64+
*/
65+
function validateMethodChecksumNoChunking(request, body, log) {
66+
if (config.integrityChecks[request.apiMethod]) {
67+
const validationFunc = methodValidationFunc[request.apiMethod];
68+
if (!validationFunc) {
69+
return null;
70+
}
71+
72+
return validationFunc(request, body, log);
73+
}
74+
75+
return null;
76+
}
77+
78+
module.exports = {
79+
ChecksumError,
80+
validateChecksumsNoChunking,
81+
validateMethodChecksumNoChunking,
82+
};

‎lib/api/bucketPutCors.js‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
const crypto = require('crypto');
21
const async = require('async');
32
const { errors, errorInstances } = require('arsenal');
43

@@ -33,16 +32,6 @@ function bucketPutCors(authInfo, request, log, callback) {
3332
return callback(errors.MissingRequestBodyError);
3433
}
3534

36-
if (request.headers['content-md5']) {
37-
const md5 = crypto.createHash('md5')
38-
.update(request.post, 'utf8').digest('base64');
39-
if (md5 !== request.headers['content-md5']) {
40-
log.debug('bad md5 digest', { error: errors.BadDigest });
41-
monitoring.promMetrics('PUT', bucketName, 400, 'putBucketCors');
42-
return callback(errors.BadDigest);
43-
}
44-
}
45-
4635
if (parseInt(request.headers['content-length'], 10) > 65536) {
4736
const errMsg = 'The CORS XML document is limited to 64 KB in size.';
4837
log.debug(errMsg, { error: errors.MalformedXML });

‎lib/api/bucketPutReplication.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ function bucketPutReplication(authInfo, request, log, callback) {
3333
requestType: request.apiMethods || 'bucketPutReplication',
3434
request,
3535
};
36+
3637
return waterfall([
3738
// Validate the request XML and return the replication configuration.
3839
next => getReplicationConfiguration(post, log, next),

‎lib/api/multiObjectDelete.js‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
const crypto = require('crypto');
2-
31
const async = require('async');
42
const { parseString } = require('xml2js');
53
const { auth, errors, versioning, s3middleware, policies } = require('arsenal');
@@ -475,16 +473,6 @@ function multiObjectDelete(authInfo, request, log, callback) {
475473
return callback(errors.MissingRequestBodyError);
476474
}
477475

478-
if (request.headers['content-md5']) {
479-
const md5 = crypto.createHash('md5')
480-
.update(request.post, 'utf8').digest('base64');
481-
if (md5 !== request.headers['content-md5']) {
482-
monitoring.promMetrics('DELETE', request.bucketName, 400,
483-
'multiObjectDelete');
484-
return callback(errors.BadDigest);
485-
}
486-
}
487-
488476
const inPlayInternal = [];
489477
const bucketName = request.bucketName;
490478
const canonicalID = authInfo.getCanonicalID();

‎lib/api/objectPutTagging.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ function objectPutTagging(authInfo, request, log, callback) {
2727
log.debug('processing request', { method: 'objectPutTagging' });
2828

2929
const { bucketName, objectKey } = request;
30-
3130
const decodedVidResult = decodeVersionId(request.query);
3231
if (decodedVidResult instanceof Error) {
3332
log.trace('invalid versionId query', {

‎tests/unit/api/api.js‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const api = require('../../../lib/api/api');
44
const DummyRequest = require('../DummyRequest');
55
const { default: AuthInfo } = require('arsenal/build/lib/auth/AuthInfo');
66
const assert = require('assert');
7+
const crypto = require('crypto');
78

89
describe('api.callApiMethod', () => {
910
let sandbox;
@@ -49,6 +50,7 @@ describe('api.callApiMethod', () => {
4950
sandbox.restore();
5051
});
5152

53+
5254
it('should attach apiMethod to request', done => {
5355
const testMethod = 'bucketGet';
5456
api.callApiMethod(testMethod, request, response, log, () => {
@@ -112,4 +114,101 @@ describe('api.callApiMethod', () => {
112114
(userInfo, _request, streamingV4Params, log, cb) => cb);
113115
api.callApiMethod('multipartDelete', request, response, log);
114116
});
117+
118+
describe('MD5 checksum validation', () => {
119+
const methodsWithChecksumValidation = [
120+
'bucketPutACL',
121+
'bucketPutCors',
122+
'bucketPutEncryption',
123+
'bucketPutLifecycle',
124+
'bucketPutNotification',
125+
'bucketPutObjectLock',
126+
'bucketPutPolicy',
127+
'bucketPutReplication',
128+
'bucketPutVersioning',
129+
'bucketPutWebsite',
130+
'multiObjectDelete',
131+
'objectPutACL',
132+
'objectPutLegalHold',
133+
'objectPutTagging',
134+
'objectPutRetention'
135+
];
136+
137+
methodsWithChecksumValidation.forEach(method => {
138+
it(`should return BadDigest for ${method} when bad MD5 checksum is provided`, done => {
139+
const body = '<xml></xml>';
140+
const headers = {
141+
'content-md5': 'badchecksum123=', // Invalid MD5
142+
'content-length': body.length.toString()
143+
};
144+
145+
const requestWithBody = new DummyRequest({
146+
headers,
147+
query: {},
148+
socket: { remoteAddress: '127.0.0.1', destroy: sandbox.stub() }
149+
}, body);
150+
151+
sandbox.stub(api, method).callsFake(() => {
152+
done(new Error(`${method} was called despite bad checksum`));
153+
});
154+
155+
api.callApiMethod(method, requestWithBody, response, log, err => {
156+
assert(err, `Expected error for ${method} with bad checksum`);
157+
assert(err.is.BadDigest, `Expected BadDigest error for ${method}, got: ${err.code}`);
158+
done();
159+
});
160+
});
161+
});
162+
163+
methodsWithChecksumValidation.forEach(method => {
164+
it(`should succeed for ${method} when correct MD5 checksum is provided`, done => {
165+
const body = '<xml></xml>';
166+
const correctMd5 = crypto.createHash('md5').update(body).digest('base64');
167+
const headers = {
168+
'content-md5': correctMd5,
169+
'content-length': body.length.toString()
170+
};
171+
172+
const requestWithBody = new DummyRequest({
173+
headers,
174+
query: {},
175+
socket: { remoteAddress: '127.0.0.1', destroy: sandbox.stub() }
176+
}, body);
177+
178+
sandbox.stub(api, method).callsFake((userInfo, _request, log, cb) => {
179+
cb();
180+
});
181+
182+
api.callApiMethod(method, requestWithBody, response, log, err => {
183+
assert.ifError(err, `Unexpected error for ${method} with good checksum: ${err}`);
184+
done();
185+
});
186+
});
187+
});
188+
189+
methodsWithChecksumValidation.forEach(method => {
190+
it(`should fail for ${method} when empty MD5 checksum is provided`, done => {
191+
const body = '<xml></xml>';
192+
const headers = {
193+
'content-md5': '',
194+
'content-length': body.length.toString()
195+
};
196+
197+
const requestWithBody = new DummyRequest({
198+
headers,
199+
query: {},
200+
socket: { remoteAddress: '127.0.0.1', destroy: sandbox.stub() }
201+
}, body);
202+
203+
sandbox.stub(api, method).callsFake((userInfo, _request, log, cb) => {
204+
cb();
205+
});
206+
207+
api.callApiMethod(method, requestWithBody, response, log, err => {
208+
assert(err, `expected error for ${method} with no checksum`);
209+
done();
210+
});
211+
});
212+
});
213+
});
115214
});

0 commit comments

Comments
 (0)