Skip to content

Commit

Permalink
[ACR] Harden downloadBlob and getManifest validation (#25769)
Browse files Browse the repository at this point in the history
### Packages impacted by this PR

- `@azure/container-registry`

### Issues associated with this PR

- Fix #25645

### Describe the problem that is addressed by this PR

Add extra validation to `downloadBlob` and `getManifest`:
- Restrict manifest size to 4 MB
- Throw early if bytes read during `downloadBlob` exceed the value in
the
  `Content-Length` header

Based somewhat on the [Java PR for the same
thing](Azure/azure-sdk-for-java#34737), but
without validation of the `Content-Range` header since we are using
optimistic download.
  • Loading branch information
timovv committed May 11, 2023
1 parent b97fcd5 commit 084e9dd
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { RetriableReadableStream } from "../utils/retriableReadableStream";
const LATEST_API_VERSION = "2021-07-01";

const CHUNK_SIZE = 4 * 1024 * 1024; // 4 MB
const MAX_MANIFEST_SIZE_BYTES = 4 * 1024 * 1024; // 4 MB

const ACCEPTED_MANIFEST_MEDIA_TYPES = [
KnownManifestMediaType.OciImageManifest,
Expand Down Expand Up @@ -254,7 +255,7 @@ export class ContainerRegistryContentClient {
assertHasProperty(response, "mediaType");

const content = response.readableStreamBody
? await readStreamToEnd(response.readableStreamBody)
? await readStreamToEnd(response.readableStreamBody, MAX_MANIFEST_SIZE_BYTES)
: Buffer.alloc(0);

const expectedDigest = await calculateDigest(content);
Expand Down
15 changes: 13 additions & 2 deletions sdk/containerregistry/container-registry/src/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,22 @@ export function isDigest(tagOrDigest: string): boolean {
return tagOrDigest.includes(":");
}

export async function readStreamToEnd(stream: NodeJS.ReadableStream): Promise<Buffer> {
export async function readStreamToEnd(
stream: NodeJS.ReadableStream,
maxLength?: number
): Promise<Buffer> {
const buffers: Buffer[] = [];
let bytesRead = 0;

return new Promise((resolve, reject) => {
stream.on("data", (chunk) => buffers.push(chunk));
stream.on("data", (chunk) => {
buffers.push(chunk);
bytesRead += chunk.length;

if (maxLength && bytesRead > maxLength) {
reject(new Error(`Stream exceeded maximum allowed length of ${maxLength} bytes.`));
}
});
stream.on("end", () => resolve(Buffer.concat(buffers)));
stream.on("error", (err) => reject(err));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ export class RetriableReadableStream extends Readable {

this.offset += data.length;

if (this.offset > this.end + 1) {
this.destroy(
new Error(
`Data corruption failure: Received more data than original request, data needed offset is ${
this.end
}, received offset: ${this.offset - 1}`
)
);
}

this.onData?.(data);
this.onProgress?.({ loadedBytes: this.offset - this.start });

Expand Down

0 comments on commit 084e9dd

Please sign in to comment.