Skip to content

Commit

Permalink
fix: check the format of the index of each attachment
Browse files Browse the repository at this point in the history
A specially crafted packet could be incorrectly decoded.

Example:

```js
const decoder = new Decoder();

decoder.on("decoded", (packet) => {
  console.log(packet.data); // prints [ 'hello', [Function: splice] ]
})

decoder.add('51-["hello",{"_placeholder":true,"num":"splice"}]');
decoder.add(Buffer.from("world"));
```

As usual, please remember not to trust user input.
  • Loading branch information
darrachequesne committed Jun 27, 2022
1 parent c7514b5 commit b5d0cb7
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 3 deletions.
12 changes: 10 additions & 2 deletions lib/binary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,16 @@ export function reconstructPacket(packet, buffers) {
function _reconstructPacket(data, buffers) {
if (!data) return data;

if (data && data._placeholder) {
return buffers[data.num]; // appropriate buffer (should be natural order anyway)
if (data && data._placeholder === true) {
const isIndexValid =
typeof data.num === "number" &&
data.num >= 0 &&
data.num < buffers.length;
if (isIndexValid) {
return buffers[data.num]; // appropriate buffer (should be natural order anyway)
} else {
throw new Error("illegal attachments");
}
} else if (Array.isArray(data)) {
for (let i = 0; i < data.length; i++) {
data[i] = _reconstructPacket(data[i], buffers);
Expand Down
3 changes: 3 additions & 0 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ export class Decoder extends Emitter<{}, {}, DecoderReservedEvents> {
public add(obj: any) {
let packet;
if (typeof obj === "string") {
if (this.reconstructor) {
throw new Error("got plaintext data when reconstructing a packet");
}
packet = this.decodeString(obj);
if (
packet.type === PacketType.BINARY_EVENT ||
Expand Down
50 changes: 49 additions & 1 deletion test/buffer.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { PacketType } = require("..");
const { PacketType, Decoder } = require("../");
const helpers = require("./helpers.js");
const expect = require("expect.js");

describe("parser", () => {
it("encodes a Buffer", (done) => {
Expand All @@ -14,6 +15,18 @@ describe("parser", () => {
);
});

it("encodes a nested Buffer", (done) => {
helpers.test_bin(
{
type: PacketType.EVENT,
data: ["a", { b: ["c", Buffer.from("abc", "utf8")] }],
id: 23,
nsp: "/cool",
},
done
);
});

it("encodes a binary ack with Buffer", (done) => {
helpers.test_bin(
{
Expand All @@ -25,4 +38,39 @@ describe("parser", () => {
done
);
});

it("throws an error when adding an attachment with an invalid 'num' attribute (string)", () => {
const decoder = new Decoder();

expect(() => {
decoder.add('51-["hello",{"_placeholder":true,"num":"splice"}]');
decoder.add(Buffer.from("world"));
}).to.throwException(/^illegal attachments$/);
});

it("throws an error when adding an attachment with an invalid 'num' attribute (out-of-bound)", () => {
const decoder = new Decoder();

expect(() => {
decoder.add('51-["hello",{"_placeholder":true,"num":1}]');
decoder.add(Buffer.from("world"));
}).to.throwException(/^illegal attachments$/);
});

it("throws an error when adding an attachment without header", () => {
const decoder = new Decoder();

expect(() => {
decoder.add(Buffer.from("world"));
}).to.throwException(/^got binary data when not reconstructing a packet$/);
});

it("throws an error when decoding a binary event without attachments", () => {
const decoder = new Decoder();

expect(() => {
decoder.add('51-["hello",{"_placeholder":true,"num":0}]');
decoder.add('2["hello"]');
}).to.throwException(/^got plaintext data when reconstructing a packet$/);
});
});
4 changes: 4 additions & 0 deletions test/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,5 +146,9 @@ describe("parser", () => {
expect(() => new Decoder().add("999")).to.throwException(
/^unknown packet type 9$/
);

expect(() => new Decoder().add(999)).to.throwException(
/^Unknown type: 999$/
);
});
});

2 comments on commit b5d0cb7

@gitDk70
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security fix

@darrachequesne
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gitDk70 this is indeed a security fix, here's the associated advisory: GHSA-qm95-pgcg-qqfq

Please # to comment.