console API on XS: compatibility, security impact? #2146

dckc · 2020-12-29T04:21:10Z

What is the Problem Being Solved?

On XS, to date, we have stubbed out the console API; for example, in #2145). This API gets exposed to user code, which is an incompatibility risk and such incompatibility risks notoriously lead to vulnerabilities.

See also UTF-8 in XS #2118

cc @kriskowal @erights @warner

Description of the Design

goals... somewhat conflicting...

never throws an error
when on chain: deterministic: don't let caller detect when you did toString() -> don't mess with args... so noop
slog
post-hoc debugging

also:

console-shim.js as of 79e81b0
- provides a console that has at least noop methods for everything in SES VirtualConsole (52e6830).
vatConsole seems to make a facet with ['debug', 'log', 'info', 'warn', 'error'] methods for use in local vat managers. Other vat managers seem to do likewise.

Security Considerations

Incompatibility risks notoriously lead to vulnerabilities

Are contract writers prepared for console.clear() to throw? Do we teach them about commit points?

Test Plan

?

The text was updated successfully, but these errors were encountered:

dckc · 2021-01-15T20:50:15Z

In the xsnap design landed this week, the console methods are all noop:
https://github.com/Agoric/agoric-sdk/blob/e775a99afae43a8581cdeedd22545c7fa703c691/packages/xs-vat-worker/src/console-shim.js

dckc · 2021-01-23T18:38:56Z

@kriskowal notes in #2225 (comment) that our vat managers provide an incomplete Console API to vats. Is this a conscious (documented) policy decision or a bug? @warner ?

add TODO re other console methods with pointer to Agoric#2146

* feat(xsnap): setImmediate and print In addition to detecting XS Machine quiescense so we can safely take snapshots, the supervisor has to detect vat queiscense so it can tell when a delivery is done. I have resorted to ad-hoc `fprintf()` at the C level for debugging enough to justify restoring print. Here we test that it's only available in the start compartment. note print() includes fflush() * build(xsnap): don't set mxDebug in release builds fixes #2216 only tested on lin, not mac nor win * build(xsnap): build GOAL=debug too * fix(xsnap): don't swallow error message * feat(xsnap): return data from xsnap.evaluate() Using the .result property of a mutable object rather than the resolution of a promise is a little awkward, but it seems to work. * chore(xs-vat-worker): prune obsolete dependencies * build(xs-vat-worker): moddable submodule is obsolete * style(xs-vat-worker): use canonical @Agoric style * feat(xs-vat-worker): TextDecoder, HandledPromise before lockdown * refactor(SwingSet): unify vat-worker filenames fixes #2202 * feat(swingset): xsnap vat manager - build xsnap bootstrap bundles - bytes to tagged array and back - setBundle, importBundle - syscall - delivery success symbol is ok, not deliverDone - Use Tagged type consistently; don't constrain tag to be string. - clean up logging: use parentLog(), trace(), ... - static typing for doProcess: capture dispatch while it's known to be not null - silence parentLog, workerLog for xsnap - no, handleSyscall doesn't return Tagged - inherit stdout, stderr in xsnap - vatid arg on doNotify is no more * fix: crank 1 comment in vat-target.js * fix: supply groupCollapsed etc. in console-shim for SES * fix(xsnap): handle edge cases in sending replies to e, ? * refactor: avoid 2nd round trip to xsnap - manager: prune commandResult - supervisor: factor out "transport" logic as `ManagerPort`, separate from vat-worker `makeWorker()` - ManagerPort.handler provides `{ result?: ArrayBuffer }` idiom based on Promise<Tagged> - testLog uses ManagerPort.send - clean up redundant 'ok' tag in doMessage, doNotify - refactor: tagged -> item for consistency * feat(xsnap worker): pass console log messages to manager - prune 'starting xsnap' log msg (per code review) - handle rejection in ManagerPort.handler * fix(xsnap): build args * refactor(xsnap): fold in what's left of xs-vat-worker - prune obsolete locate.js * chore(xsnap): move lockdown-shim out of src/ to avoid tsc errors move lockdown-shim.js and the rest of the SES bootstrap files from src/ to lib/ to avoid many tsc errors of the form... ``` Error: ../../node_modules/ses/src/error/assert.js(24,20): error TS2304: Cannot find name 'StringablePayload'. ``` * docs(xsnap): document XS handleCommand async idiom * refactor: build XS bundles with Kernel bundles * fix(xsnap worker): update syscall API to use .resolve() * chore(xsnap): provide non-trivial console in start compartment add TODO re other console methods with pointer to #2146

dckc · 2021-01-25T22:25:34Z

notes from chat with @warner ...

goals... somewhat conflicting...

never throws an error
when on chain: deterministic: don't let caller detect when you did toString() -> don't mess with args... so noop
slog
post-hoc debugging

This merits further discussion. But it's not clear that there's a next step for me, so I'm removing my assignment.

dckc · 2022-04-06T23:38:30Z

I think the use of object-inspect should move this into MN-1.

dckc · 2022-05-27T17:52:13Z

@turadg should we re-open this? well, I guess this issue is about XS, and what bit you is the SwingSet vat console:

agoric-sdk/packages/SwingSet/src/supervisors/supervisor-helper.js

Lines 158 to 170 in 9cb480c

    
            * TODO: consider other methods per SES VirtualConsole. 
        
            * See https://github.com/Agoric/agoric-sdk/issues/2146 
        
            * 
        
            * @param {(level: string) => (...args: any[]) => void} makeLog 
        
            */ 
        
           function makeVatConsole(makeLog) { 
        
             return harden({ 
        
               debug: makeLog('debug'), 
        
               log: makeLog('log'), 
        
               info: makeLog('info'), 
        
               warn: makeLog('warn'), 
        
               error: makeLog('error'), 
        
             });

#5429 (comment)

turadg · 2022-05-27T18:17:37Z

I think it needs an open issue. Whether you reopen this one or I file a new one, up to you.

Acceptance criteria

the console type while coding in agoric-sdk matches the available console object during code execution

That could be by making Swingset's console support the console API or by modifying the typedef for the global console. I'd prefer to support the API as much as possible (if even with throwing "not implemented") than to create more special cases.

dckc · 2022-05-27T18:40:03Z

please open a new one, then, @turadg

turadg · 2022-05-27T18:51:48Z

👍 #5453

dckc added the enhancement New feature or request label Dec 29, 2020

dckc added needs-design security and removed enhancement New feature or request labels Jan 15, 2021

dckc self-assigned this Jan 15, 2021

dckc mentioned this issue Jan 15, 2021

XS deep freeze conflicts with SES security constraints #1058

Open

dckc mentioned this issue Jan 23, 2021

feat(swingset): xsnap vat worker #2225

Merged

7 tasks

dckc added a commit to dckc/agoric-sdk that referenced this issue Jan 24, 2021

chore(xsnap): provide non-trivial console in start compartment

37a658c

add TODO re other console methods with pointer to Agoric#2146

dckc assigned warner and erights Jan 24, 2021

erights mentioned this issue Jan 24, 2021

Add missing console methods #2254

Merged

dckc removed their assignment Jan 25, 2021

dckc added the SwingSet package: SwingSet label Jan 25, 2021

dckc added the xsnap the XS execution tool label Apr 28, 2021

dckc added this to the XS Memory Safety Audit milestone Jul 21, 2021

dckc removed this from the XS Memory Safety Audit milestone Aug 3, 2021

dckc mentioned this issue Feb 8, 2022

liveSlots consensus mode and better verbosity switches #4364

Merged

erights added security-integrity and removed security labels Feb 15, 2022

michaelfig mentioned this issue Mar 8, 2022

Rework xsnap/src/object-inspect.js #4767

Merged

dckc mentioned this issue Mar 10, 2022

review or replace object-inspect.js used by xsnap console #4814

Closed

dckc removed the needs-design label Apr 6, 2022

Tartuffo added security and removed security-integrity labels Apr 8, 2022

mergify bot closed this as completed in #4767 Apr 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console API on XS: compatibility, security impact? #2146

console API on XS: compatibility, security impact? #2146

dckc commented Dec 29, 2020 •

edited

Loading

dckc commented Jan 15, 2021

dckc commented Jan 23, 2021

dckc commented Jan 25, 2021 •

edited

Loading

dckc commented Apr 6, 2022

dckc commented May 27, 2022 •

edited

Loading

turadg commented May 27, 2022

dckc commented May 27, 2022

turadg commented May 27, 2022

console API on XS: compatibility, security impact? #2146

console API on XS: compatibility, security impact? #2146

Comments

dckc commented Dec 29, 2020 • edited Loading

dckc commented Jan 15, 2021

dckc commented Jan 23, 2021

dckc commented Jan 25, 2021 • edited Loading

dckc commented Apr 6, 2022

dckc commented May 27, 2022 • edited Loading

turadg commented May 27, 2022

dckc commented May 27, 2022

turadg commented May 27, 2022

dckc commented Dec 29, 2020 •

edited

Loading

dckc commented Jan 25, 2021 •

edited

Loading

dckc commented May 27, 2022 •

edited

Loading