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

SharedArrayBuffer timer to get network timings #75

Merged
merged 20 commits into from
Dec 10, 2020
Merged

SharedArrayBuffer timer to get network timings #75

merged 20 commits into from
Dec 10, 2020

Conversation

NDevTK
Copy link
Contributor

@NDevTK NDevTK commented Nov 12, 2020

@NDevTK NDevTK mentioned this pull request Nov 12, 2020
Copy link
Member

@terjanq terjanq left a comment

Choose a reason for hiding this comment

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

I haven't heard of Atomics API 😮 . Maybe we should introduce it in clocks article? What do you think?

One objection I have about this PR is that it adds a significant portion of code snippet to the network timing article that is already one of the longest in the wiki. While we want the snippets to be accurate as possible, the main goal of them is to reflect what we write in plain text into the actual code, or if the snippet is simple enough, accompany it with extensive comments as a replacement.

Instead of writing a working Proof of Concept, I would be leaning more towards presenting the concept and describing it in more depth.

We plan on creating Proof of Concepts in the form of interactive examples in the future which will be focused on making them as accurate as possible, so this amazing job that you are doing should not go to vain. But for the initial iteration I feel like this is a little bit too nuanced.

@NDevTK
Copy link
Contributor Author

NDevTK commented Nov 14, 2020

@terjanq I made some changes. do you think https://github.com/cgvwzq/spectre/blob/master/spectre.js#L89 helps with the timer? I dont know what it does.

@NDevTK NDevTK requested a review from terjanq November 14, 2020 17:15
Copy link
Member

@terjanq terjanq left a comment

Choose a reason for hiding this comment

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

I added a few changes in cd746a8 so don't forget to pull before making further changes.

I really like the improvements you did to the sharedArray clock, it is cleaner than it was before, and I also like that you used it with a new technique to showcase how this clock can be used!

content/docs/attacks/timing-attacks/network-timing.md Outdated Show resolved Hide resolved
@terjanq
Copy link
Member

terjanq commented Nov 16, 2020

@terjanq I made some changes. do you think https://github.com/cgvwzq/spectre/blob/master/spectre.js#L89 helps with the timer? I dont know what it does.

I am not sure either, but it looks like the spectre attack nuance that clears the memory segments.

@NDevTK
Copy link
Contributor Author

NDevTK commented Nov 16, 2020

I tested the difference between performance.now() and sharedArray
The duration difference is higher for the sharedArray clock but both seems to work

@NDevTK NDevTK requested a review from terjanq November 16, 2020 19:58
@NDevTK
Copy link
Contributor Author

NDevTK commented Nov 19, 2020

@terjanq it seems this improves reliability for requests with smaller size differences

async function getDuration2(url) {
    var best = null;
    // Find the lowest duration possible
    for (let i = 0; i < 10; i++) {
        let result = await getDuration(url);
        await new Promise(resolve => (iframe.onload = resolve));
        if (best === null || result < best && result > 0) best = result;
    }
    return best
}
// Or the average
async function getDuration3(url, count = 10) {
    var total = null;
    for (let i = 0; i < count; i++) {
        let result = await getDuration(url);
        await new Promise(resolve => (iframe.onload = resolve));
        total += result;
    }
    return total/count
}

Copy link
Member

@terjanq terjanq left a comment

Choose a reason for hiding this comment

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

I added a few suggestions. There will be conflicts when merging to master since I updated the defense tables in the meantime. Also, we should update the defense table for the new attack with Unload Events. I believe the table would look something like:

Unload Events:
SameSite Lax: X^1 (in footnote 1: SameSite Lax could protect against the attack inside an iframe but would not top-level navigations)
Framing Protections: X
COOP: X
Isolation Policies: NIP^2 (in footnote 2: FIP could protect against the attack inside iframes, but wouldn't protect against new windows)

content/docs/attacks/timing-attacks/network-timing.md Outdated Show resolved Hide resolved
Comment on lines 141 to 154
{{< hint tip >}}
Network performance changes may make it harder to derive the transfer size.
You can do something like the following to get the duration with the best network performance.
```javascript
var best = null;
// Find the lowest duration possible
for (let i = 0; i < 10; i++) {
let result = await getDuration(url);
await new Promise(resolve => (iframe.onload = resolve));
if (best === null || result < best && result > 0) best = result;
}
```
{{< /hint >}}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{< hint tip >}}
Network performance changes may make it harder to derive the transfer size.
You can do something like the following to get the duration with the best network performance.
```javascript
var best = null;
// Find the lowest duration possible
for (let i = 0; i < 10; i++) {
let result = await getDuration(url);
await new Promise(resolve => (iframe.onload = resolve));
if (best === null || result < best && result > 0) best = result;
}
```
{{< /hint >}}

I would personally drop this part. This applies to other measurements as well, not only Unload events section.

Copy link
Contributor Author

@NDevTK NDevTK Dec 10, 2020

Choose a reason for hiding this comment

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

Removed it. could it be moved?

Copy link
Member

Choose a reason for hiding this comment

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

Potentially yes, but not sure what is the best place yet. Maybe in the future, when (if) we have a section about making the attacks more reliable ?! Very unsure what is the place for it now, and whether it improves the contents significantly

NDevTK and others added 2 commits December 10, 2020 17:01
Co-authored-by: terjanq <terjanq@users.noreply.github.com>
Co-authored-by: terjanq <terjanq@users.noreply.github.com>
Co-authored-by: terjanq <terjanq@users.noreply.github.com>
@terjanq
Copy link
Member

terjanq commented Dec 10, 2020

Merging, will update the defense table later.

@terjanq terjanq merged commit bdd3431 into xsleaks:master Dec 10, 2020
@NDevTK NDevTK deleted the patch-3 branch December 10, 2020 18:00
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.

None yet

2 participants