overlookmotel / livepack

Serialize live running code to Javascript
MIT License
30 stars 0 forks source link

Check have info for correct function when serializing functions #550

Closed overlookmotel closed 7 months ago

overlookmotel commented 7 months ago

If there's a bug in Livepack which causes tracker not to execute when serializing a function, this can produce incorrect output rather than an error.

For example, currently there's a known issue with classes with properties potentially having side effects when attempting to serialize them (#549).

Input:

const f = function() {
  console.log('yo!');
};
export default class X {
  foo = f();
};

This is instrumented as:

const livepack_scopeId_2 = livepack_getScopeId();
const f = function () /*livepack_track:3;f;/src/index.js*/ {
  livepack_tracker(livepack_getFnInfo_3, () => []);
  console.log('yo!');
};
module.exports = class /*livepack_track:7;c;/src/index.js*/ X {
  foo = f();
  constructor() {
    livepack_tracker(livepack_getFnInfo_7, () => [[livepack_scopeId_2, f]]);
  }
};

When Livepack calls new X(), it intends to trigger the tracker in X's constructor, but instead class property foo is evaluated first. That calls f() which executes the tracker on function f instead.

So output is:

export default function X() {
  console.log('yo!');
}

Side effects when serializing functions is a serious problem. The code which is executed could do anything, and in worst case could be a security vulnerability if an attacker was able to leverage it in order to execute code which is only meant to execute on the client on a server at build time.

It isn't possible to prevent the code execution without fixing the bug which is the source of the problem (#549 in this case). However, can at least make sure it throws an error rather than silently succeeding.

This is not specific to the class properties bug. The point is that there might be other bugs which also cause side effects when serializing functions which are as yet unknown, and it's important to surface these bugs as soon as possible.

One solution would be to call the tracker with the function ID and file path:

class /*livepack_track:7;c;/src/index.js*/ X {
  constructor() {
    livepack_tracker(
      7, // <-- Function ID
      '/src/index.js', // <-- File path
      livepack_getFnInfo_7,
      () => [[livepack_scopeId_2, f]]
    );
  }
};

If the function ID and file path the tracker are called with don't match what's in the tracker comment, then the wrong tracker has executed.

Alternatively, could put the function ID and file path in the livepack_getFnInfo functions, but would have to make sure livepack_getFnInfo is always called, not just first time the function is serialized.

overlookmotel commented 7 months ago

549 is now fixed.

I have over-stated the security implications of bugs in this area. Livepack runs code at build time anyway, so there are far easier ways for an attacker to illicitly execute arbitrary code than exploiting bugs like this.

Class properties were a special case, and I am fairly confident that there are no other ways remaining for serializing a function to accidentally execute code. Therefore, adding a failsafe check would just be bloat.