Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export copy to clipboard functionality #50

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Export copy to clipboard functionality #50

wants to merge 14 commits into from

Conversation

weizman
Copy link
Member

@weizman weizman commented Jul 12, 2024

close #46 by providing a new LavaDome method copy which copies the secret to clipboard without leaking it to any other entity.

@weizman Don't forget to address TODO in code and update docs

@weizman weizman marked this pull request as draft July 12, 2024 23:01
@weizman weizman marked this pull request as ready for review July 16, 2024 11:38
@weizman weizman self-assigned this Jul 25, 2024
@weizman weizman force-pushed the main branch 2 times, most recently from 7958a69 to fa4643f Compare September 18, 2024 08:04
@weizman weizman requested review from legobeat and naugtur September 18, 2024 08:28
@@ -41,21 +46,25 @@ export function LavaDome(host, opts) {
const child = hardened();
appendChild(shadow, child);

function text(text) {
if (typeof text !== 'string') {
let secret = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't feel great to have the secret hang out in memory in this long-lived scope... Not that I have a proposal on how it could be achieved differently, though...

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point, but essentially, it's only natural that in order to provide service where the input is somehow manipulated we must have access to it

defineProperties(this, {text: {value: text}});
defineProperties(this, {
text: {value: text},
copy: {value: copy},
Copy link
Collaborator

@legobeat legobeat Oct 1, 2024

Choose a reason for hiding this comment

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

Re https://github.com/LavaMoat/LavaDome/pull/50/files#r1782176796

Could setting the value of copy (including its declaration and instantiation) be moved to the inner scope of text? If so, maybe the need to move secret out of scope is no longer here?

Or would that conflict with the "make exported API tamper-proof" part? If so I think I'm still leaning towards that as the lesser bad. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would we export copy if it's inside text?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we assign it inside the handler of text, is there a need to?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIU, if you move the copy function into the text function, you closure the secret within the text scope instead of the LavaDome scope, which doesn't solve the problem, just shifts it elsewhere - the secret still must be scoped somewhere

Copy link
Collaborator

@legobeat legobeat Oct 1, 2024

Choose a reason for hiding this comment

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

The secret is already part of the text scope (by virtue of being an argument).

However, moving/copying it to the outer scope widens the exposure (and lifetime, but not sure to what extent we can actually limit that if introducing the copy-to-clipboard functionality as intended).

For example, consider a scenario where text() is called a second time with invalid input on an existing LD instance:

ld.text('actualsecret');
// at some point later...
ld.text();

This will throw an Error where 'actualsecret' is still in scope. I don't have a PoC or anything but does seem like it could more easily leak to a malicious extension or devtool/debugger integration.

Vs if it's contained in the text function, separate text() invocations are not able to access each others' secrets, and subsequent calls would make new copy instances with isolated scopes wrt secret.

Copy link
Collaborator

@legobeat legobeat Oct 2, 2024

Choose a reason for hiding this comment

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

One way to at least prevent one aspect of scope-creep of secret itself without compromising on the tamper-proofing could be to freeze a property accessor and reassign copy itself instead:

// instead of `let secret`
let _copy = () => Promise.resolve();

async function copy() {
  await _copy();
}

// [...]

function text(input) {
  _copy = async () = {
    // `_copy` can now access `input` from `text` directly
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

sold c8c9cd7

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually just removed c8c9cd7 because:

  1. it turns out the way i did it dropped a commit coming from main by accident which could have been a terrible mistake
  2. a younger comment made me realize the implementation isn't right, and that also I'm again not convinced this can be solved without storing the secret in some upper scope

For example, I'm not sure how #50 (comment) demonstrates this otherwise, how scoping a function that has access to the secret instead of the secret itself is far better?

A demonstrating PR here could very much help me understand 🙏

throw new Error(
`LavaDomeCore: first argument must be a string, instead got ${stringify(text)}`);
`LavaDomeCore: first argument must be a string, instead got ${stringify(input)}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`LavaDomeCore: first argument must be a string, instead got ${stringify(input)}`);
`LavaDomeCore: first argument must be a string, instead got ${typeof input}`);

Type should be enough, no need to template secret in error message

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

also 38ef179

Comment on lines +80 to +83
const type = 'text/plain';
const blob = new Blob([secret], {type});
const data = [new ClipboardItem({[type]: blob})];
await write(clipboard, data);
Copy link
Collaborator

@legobeat legobeat Oct 2, 2024

Choose a reason for hiding this comment

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

Maybe swallow errors on write?

Suggested change
const type = 'text/plain';
const blob = new Blob([secret], {type});
const data = [new ClipboardItem({[type]: blob})];
await write(clipboard, data);
try {
const type = 'text/plain';
const blob = new Blob([secret], {type});
const data = [new ClipboardItem({[type]: blob})];
// do we also want to return the result of the await here?
await write(clipboard, data);
} finally {}

Copy link
Member Author

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To reduce the likelihood of an error containing a reference to the secret?

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't swallow errors that have nothing to do with us, the app should become aware of such errors being thrown. The error will never leak the secret, because if it did, it would have been a major info leak in the design of the native API, which should remain outside of LavaDome's jurisdiction.

@weizman weizman requested a review from legobeat October 7, 2024 13:51
boneskull
boneskull previously approved these changes Oct 7, 2024
Copy link

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

mostly just questions. I'm not familiar with this (or React)

// add a distraction against side channel leaks attack attempts
appendChild(child, distraction());
}
}

export function LavaDome(host, opts) {

Choose a reason for hiding this comment

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

Is this a constructor? A React component? if the former, why not use class?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the former.
Because there's no way to make methods of a class (truly) private and inaccessible.

Copy link

@boneskull boneskull Oct 8, 2024

Choose a reason for hiding this comment

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

not even with #private? but wait, why do we need to worry about that at all in this use case? I don't see any methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

not even with #private
Screenshot 2024-10-09 at 11 31 55

Copy link
Member Author

Choose a reason for hiding this comment

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

text and the new copy are methods.
They're not assigned to the prototype as methods for the same privacy reason, but they are effectively methods of the instance.

packages/core/src/native.mjs Show resolved Hide resolved
}

if (!hasOwn(textToTokenMap, text)) {
const token = create(null);
textToTokenMap[text] = token;
set(tokenToTextMap, token, text);
set(tokenToCopyInvokerMap, token, () => {});

Choose a reason for hiding this comment

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

why is there a noop function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just generally in favor of having default values correspond to expected values, so since the values of this map are the copy method which is a function, I figured the default value should also be a function.

Am open to suggestions.

}

return textToTokenMap[text];
const token = textToTokenMap[text];
const copy = () => get(tokenToCopyInvokerMap, token)();

Choose a reason for hiding this comment

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

should this be a bound function or does it not matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

the copy function stores what it needs within the closure that was originally constructed for it, which is the alternative to bounding in this case

// variable @text is named that way only for visibility - in reality it's a lavadome token
const token = text, host = useRef(null);
export const LavaDome = ({ token, unsafeOpenModeShadow }) => {
const host = useRef(null);

Choose a reason for hiding this comment

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

what does this do?

Choose a reason for hiding this comment

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

to be clear, I'm asking about L60 not L59

Copy link
Member Author

Choose a reason for hiding this comment

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

useRef is this React feature that allows you to create a reference to some value that should not trigger rerendering of a React component (since other types of variables definition in React do trigger a rerender).

It's useful here, because React allows you to make it point to a non React declarative DOM node, so that it's accessible when passed on to another component (which is what we do with the span host here).


return (
// form a span to act as the LavaDome host
<span ref={host}>
<LavaDomeShadow
host={host} token={token}
host={host}
// accept both formats (token, or [token, copy])

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally, so that users can ignore the copy option and just do const token = toLavaDomeCapabilities(secret);, but I think to cancel that would mean they'd have to do const [token] = toLavaDomeCapabilities(secret); which is fine, so i need to see if that's the only reason which makes this a wrong choice or not

Choose a reason for hiding this comment

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

would prefer to do [token]/[token, copy] if there's no great reason just for simplicity sake. I assume this is a React thing that means you can't just use an object and instead need to deal with arrays

Copy link
Member Author

Choose a reason for hiding this comment

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

c9f2ec4 (you're right about the object part too)

replaceChildren(shadow);
function defineCopy(instance, input) {
return async function copy() {
const type = 'text/plain';

Choose a reason for hiding this comment

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

how can we support other mimetypes?

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole project currently focuses on plain text only because of how hard it is to safely present more complex types.

Generally, this is a question for another day, but i might be able to answer your question if you have an example in mind for "other mimetypes".

Choose a reason for hiding this comment

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

a PNG. say it's a QR code for initializing TOTP that we don't want to expose

Copy link
Member Author

Choose a reason for hiding this comment

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

Very interesting, perhaps sometime far in the future

return function text(input) {
const type = typeof input;
if (type !== 'string') {
throw new Error(

Choose a reason for hiding this comment

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

Suggested change
throw new Error(
throw new TypeError(

look, an actual suggestion!

Copy link
Member Author

Choose a reason for hiding this comment

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

damn right fb55997

const span = createElement(document, 'span');
// mark as internal LavaDome instance
opts[OPTIONS.isInnerInstance] = true;
new LavaDome(span, opts).text(char);
appendChild(child, span);
});

// make exported API tamper-proof
defineProperties(instance, {
copy: {value: defineCopy(instance, input)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this error with Cannot redefine property: copy if text is called twice on the same instance? Could we add testing coverage for what happens in that scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

outdated, continue @ #50 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support copy to clipboard
3 participants