V1 Audit information

Some more detailed information:

We have a kick-off call Monday, September 30th at 3 PM EDT with Trail of Bits to start the audit. The scope is the same as the OP, here are the engineer details:

  • Sefan Edwards and Sam Moelius will begin the review going 9/30 - 10/11
  • Josselin, Sam, and Dominik will then complete the review from 10/15 - 11/1

Each week will have a summary report which will allow us to start working on found issues during the audit. I hope to set up a channel in the new discord with all auditors and relevant Status CCs for quick communication on issues found.

I’m working on their plans w.r.t. @rachel’s question about branch management. Will update with details there when I get them.

1 Like

So for comms during the Audit, ToB would like to create a specific channel in their slack and invite members to it. There reasoning is a-few-fold:

  • they can bring in resources of experience easily when needed, as everyone in the company uses that slack as their main comms
  • they want to maintain chat history of the review
  • they asked first

The question now is who would like to be included into this? Your goals would be to keep up with the discussion and real-time findings, ask appropriate questions as well as answer any clarification needs they may have when doing things.

I know we all LOVE having extra chat clients open and watching them, but this is a short term thing, and quite important.

Might be overstepping my bounds here, but this is how I’ve always done branches and it’s always worked well:

  • master is a branch from where you can always run the latest and greatest. It should also always be green. On the case of GitHub it needs to be a protected branch.
  • branches and WIP PRs are where the magic happens. I like to push early and often and open PRs as soon as possible to start a discussion around what may well be wrong implementation.
  • tags should be used judiciously and should be immutable. Anything that requires a specific version on any point in time can work off of a tag, knowing it will always be the same.
  • develop branches are mostly an anti-pattern, but I could be convinced otherwise.
1 Like

Can you elaborate on the tags part of things.

In the past audits with ToB, they cloned a repo to a private repo within the github.com/trailofbits org to keep discussion private until we publicize the finalized report. The only thing here would be to decide which exact branches they do this with, or if it’s even a reasonable thing to do.

Tags are a snapshot at a specific point in time, so they’re perfect for this use case. “We need you to help us test v1.0.5” means they can check out the v1.0.5 tag and know that will never change.

This way they can use our repository, and we can create a separate private repo under the status-im organization where only issues are created to track security concerns (and, as a result, we control access so we can invite whomever we find pertinent.)

Edit: I should say that tags should be used as a snapshot as a specific point in time. As long as we treat tags as immutable, we’re good.

TL;DR: We haven’t decided which commits/tags the security audit is being based on. Any opinions on this?

We had a good introductory meeting with Trail of Bits today.

As you all know, there will be a security audit in the coming month that will focus on these repositories:

For now, the team is grabbing the latest HEAD and will get setup from that. Do you have strong opinions or ideas on which commit/tag they should base their security tests from?

Pinging @rachel @pedro @andrey @cammellos

2 Likes

For status-protocol-go HEAD is fine, some parts of the codebase are not used by v1 by status-react, only by status-console-client (different client), so those strictly speaking don’t need to be audited, I can provide more details if necessary.

Thank you, @cammellos! I think testing the whole thing is a good idea (we don’t know what we don’t know), but I’d refer to @petty on that.

How much of the codebase isn’t used? Can you give me some more details on that? Hard to say without more info.

Most of it it’s used by both, but basically any parsing of the transit message is not used in status-react:

Basically for status-react this condition is always false:

Any code under that is only used by status-console-client. The plan is to have status-react use that code as well, but won’t be in v1 for sure.

2 Likes

based on this, I’d rather keep it in scope. So we’ll keep the entirety of the repo there.

1 Like

To make sure that we test exactly what we’re shipping, I agree it’s best to work backwards from status-react in terms of SHAs. As such, the following SHAs were picked:

If anyone disagrees with any of this, please speak up as it’s more of an educated guess than anything else.

3 Likes

status-im/status-react : 945ce4 (last commit pushed in the chore/store-eip1581 branch, PR #9096)

Not sure I understand this, but the commit above is not even merged in the develop branch nor is ready, it turns out there’s more work to do as we currently store the master key and that needs changing. (This might require a change in status-go as well as we need to store the address of the key 1581).

Status-protocol-go commit looks ok

That’s my bad. I thought, as it had been approved, that it was good to go.

As mentioned in channel, storing the new key path has security implications that need to be considered in the audit, so we 100% have to include this functionality.

Not trying to put pressure, but do you have an estimate on when we can give Trail of Bits a commit hash to work from?

The PR is ready now https://github.com/status-im/status-react/pull/9096 , I need some input from someone from the keycard team, as I don’t own one myself and can’t test the functionality with the keycard (as far as I can see, it should still work, but without testing it I can’t be sure), no change in status-go was necessary.

The commit hash can only be used though once merged in develop, as there might be some extra changes + we rebase against it, so it’s likely to change.

cc @guylouis @dmitryn If you have some time to look over the PR

1 Like

I’ve asked ToB about whether any of this hinders their work, will respond here when they answer. FWIW, They usually start work late morning EDT time.

It sounds like there’s another dependency implication for the audit. There’s a PR which updates status-im/status-go to use Geth 1.9.5 that needs merging, and I’m trying to understand what the testing/time implications of this are.

Will keep the thread updated as I know more.

The eip1581 PR has now been merged: https://github.com/status-im/status-react/commit/ba34af0dd45fa256cef59ba640a7eff524dcfef4

3 Likes

biometric has been merged as well https://github.com/status-im/status-react/commit/b3d04bb68c1a11a6d901c30f588c882d3f5fd530

3 Likes

Thank you @cammellos and @andrey! We’re just waiting on the Geth 1.9.5 bump and will update TofB with the new commit hashes.