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

Document potential "Missing Required Modifications" #826

Closed
defuse opened this Issue Apr 4, 2016 · 11 comments

Comments

@defuse
Contributor

defuse commented Apr 4, 2016

We are all keenly aware that there might be security bugs in our code. This is why we spend time reviewing pull requests and plan to get security auditors looking at all the modifications we've made to bitcoin core. However, I feel there's a class of bugs that isn't getting enough of our explicit attention.

I foresee three classes of mistakes:

  1. A mistake in the code we've added to bitcoin core, which breaks the security of the Zcash system.
  2. A mistake in one of our modifications to bitcoin core, which breaks the security of some system that already exists in bitcoin (p2p protocol, wallet, consensus), even if users never do any zero-knowledge transactions.
  3. Changes that we were supposed to make, but didn't, either because we didn't even consider making those changes, or we ran out of time.

Our team is already well aware of bug types (1) and (2). It's (3) that I'm worried about. It's worrying because (3) includes "unknown unknowns." By (3) I mean changes we need to make but aren't even aware of right now. For example changing the alert broadcast private keys (#424), or dealing with the block heights that are hard-coded into upstream (#420). If we had forgotten about either of those things, we would have shipped a product that gave someone else access to our alert system, or a product that suddenly dies once a certain block height is reached.

What else are we forgetting? There are almost certainly other changes we need to make that we aren't even aware of right now.

I suggest we have a separate audit for type-(3) bugs. Our internal security audit and external security audits are probably going to focus on type-(1) and type-(2) bugs without thinking of type-(3) bugs. We need time set aside to think about type-(3) bugs.

@daira daira added this to the Consensus Code Security Freeze milestone Apr 4, 2016

@defuse

This comment has been minimized.

Contributor

defuse commented Apr 4, 2016

Concretely, close this issue by having a meeting to brainstorm possible type-(3) issues.

@daira

This comment has been minimized.

Contributor

daira commented Apr 4, 2016

Let's do the meeting to brainstorm this well in advance, say within the next couple of weeks.

@defuse

This comment has been minimized.

Contributor

defuse commented Apr 4, 2016

It would be useful to collaborate with upstream developers on this, if possible.

@defuse

This comment has been minimized.

Contributor

defuse commented Apr 21, 2016

We can do the brainstorming asynchronously. I don't think well in meetings anyway.

Here's my brainstorm:

  • "Names" in the codebase that refer to external things specific to bitcoin that we should also be duplicating. For example, URLs to the GitHub repo (#884) or to the bitcoin website. Names of IRC channels in documentation, etc.
    [bitcartel: related to #884 and #1166]
    [bitcartel: there's a whole bunch of them... https://github.com/zcash/zcash/search?utf8=%E2%9C%93&q=github.com%2Fbitcoin&type=Code ]
  • Public values corresponding to secret values held by bitcoin people, e.g. alert broadcast keys [bitcartel: fixed by #424] Anything else?
  • Things that create undesired interaction between the bitcoin network and our network, e.g. leads to zcash nodes trying to connect to bitcoin nodes, or leads to zcash nodes' IP addresses being broadcast on the bitcoin network, or things like that.
    [bitcartel: protocol version bump and network message magic bytes should prevent that]
    [arcalinea: opened #1151]
  • Infrastructure that's required for the P2P network to work, which we haven't needed because we're connecting directly to our testnet. For example, DNS seeds, the thing where it attempts to connect to .onions.
    [bitcartel: infrastructure to create a dns seed and/or another mechanism such as http seeding via the cartographer protocol as used by bitcoinj]
    [arcalinea: now using a DNS seeder, see issue #630]
  • Time- or blockheight-based modifications of behavior, e.g. special casing of block numbers when new features were introduced, etc.
  • Parameter tuning: Zcash is different than bitcoin, so any tweakable knobs upstream bitcoin has set may need to be set differently for Zcash.
    [arcalinea: opened #1152]
  • Stuff that may have been important in 2009 for bootstrapping the original Bitcoin network which has since been stripped out of the code because it's already a huge network and it doesn't need that anymore.
    [arcalinea: addressed in #630 ]
  • Interaction between bitcoin configuration files and zcash configuration files (confusing one for the other, etc.).
    [arcalinea: addressed in #844]
  • Usability interactions. For example, if it's easy to confuse bitcoin addresses for zcash addresses and accidentally destroy value by sending money to the wrong type of address. Similarly for other user-facing things, like user-facing encodings of secret keys, if the three letters "BTC" are shown after any value amounts in the UI or docs, that will be confusing.
    [arcalinea: zcash address prefixes were changed. but much of the documentation remains unchanged from Bitcoin, and "BTC" may still appear in code.]
  • Web-based "is your version up to date?" checks. Automatic updates?
    [bitcartel: not in 1.0, we can search for any existing update checking.]
  • Block height soft maximums?
    [bitcartel: needs clarification from taylor]
  • All of these bullet points rewritten with litecoin (and all other bitcoin forks) in place of bitcoin. (We need to limit ourselves here, we can't defend against a hypothetical altcoin that searches and destroys zcash private keys, since at that point it's malware on the users' system.
    [bitcartel: needs clarification from taylor]
  • Forgetting to update the default transaction prioritization algorithm for the new features (maybe we want to prioritize pours over cleartext?)
    [bitcartel: related to #1133 , clarification from Taylor]
  • Adjusting the "coinbase can be spent after" depth if we change the block interval.
    [bitcartel: see #953 and #952]
    [arcalinea: update: In #952 we decide to go with 6 confirmations, although that takes less time than in Bitcoin. In #953 we leave Coinbase maturity threshold unchanged.]
  • Forgetting to add zcash-specific secrets to the output of the backupwallet command, and people expect that to be sufficient for backing up their Zcash wallet when it isn't. (Think about other RPC calls that need to be updated, too. There are a lot that may not be relevant anymore e.g. the hd-* ones. There are some for converting between hex / WIF formats etc. dumpwallet, etc.).
    [bitcartel: related to RPC]
  • Wallet Import Format (WIF) stuff.
    [bitcartel: related to RPC]
  • With bitcoin you can download a partial blockchain with bittorrent to get your node going. Do we want to host a torrent for zcash?
    [arcalinea: opened issue #1153. update: closed as downloading torrent now takes longer than letting Bitcoin Core sync]
  • Does any of our changes affect the transaction chaining / orphan logic (require changes to it)?
    [bitcartel: circle back with taylor]
    [arcalinea: tx rebroadcast mechanism from Bitcoin will not work for joinsplits in case of blockchain reorg, is this what he means?]
  • Forgetting to get rid of the "extra nonce solution" (the thing because the nonce parameter was too small)?
    [bitcartel: circle back with taylor]
  • Mining protocols (e.g. GetWork) which might be implemented somewhere in Core which are now incorrect?
    [bitcartel: GetWork is no longer used upstream, GetBlockTemplate is used for miners]
    [arcalinea: See #1424 for changes to GetBlockTemplate]
  • Things in bitcoin documentation about their governance model which aren't true about our governance model (and other such things).
    [arcalinea: opneed #1154]
  • Not having P2P distribution of the proving parameters, might overload our webserver (or huge S3 bill since they're on S3).
    [bitcartel: related to #416]
    [arcalinea: opened #1155. update: Not in 1.0.]

(I plan to edit this comment with more ideas).

For 1.0 it's probably fine if we release with most of these things, like website URLs, unfixed. We can restrict our focus to (1) zcash nodes don't ever communicate with bitcoin nodes (2) after seeing it works for a little while we can be confident it will keep working (e.g. no logic bombs like hard-coded block heights) (3) confidential secrets are changed, (5) no severe usability confusion failures. Anything else?

(For this brainstorm I read through the ToC and index sections of "Mastering Bitcoin", next step is to scan through the code)

@jcb82

This comment has been minimized.

jcb82 commented May 2, 2016

It looks like there is a quite comprehensive list going so far. I'm not sure I can venture specific additions since I don't know the codebase well enough. I might suggest the following steps to try to be more systematic:

*An audit of all #defined constants in the source code. This is a primary source of things that might have been forgotten to be updated.
*An audit of all strings in the source code for similar reasons.

@defuse

This comment has been minimized.

Contributor

defuse commented Jun 9, 2016

Here's another example (from main.cpp). The comment will become incorrect once we change the block interval:

    // If our best fork is no longer within 72 blocks (+/- 12 hours if no one mines it)
    // of our head, drop it
    if (pindexBestForkTip && chainActive.Height() - pindexBestForkTip->nHeight >= 72)
        pindexBestForkTip = NULL;
``

@nathan-at-least nathan-at-least modified the milestones: z9 Release - RPC, Audit tasks, Testing improvements, Audit Tasks Aug 8, 2016

@daira

This comment has been minimized.

Contributor

daira commented Sep 4, 2016

Add Zcash-specific safe calls to the calls allowed in safe mode.

#1337 (comment)

[Edit: I checked for rc2 that this hadn't been forgotten.]

@defuse

This comment has been minimized.

Contributor

defuse commented Sep 4, 2016

Notes from our collective brainstorm (which happened a long time ago; I just recently found the notes I took from the meeting):

  • Dust -- turn it off?
  • Pruning / Joinsplit -- Make sure we don't prune stuff we still need (UTXO are spent but still have pours or something)
  • Would be nice to simulate launch + activity for x years into the future.
  • Split up research work and recombine in a new meeting (I can't remember what this means).
  • Interaction with the Bitcoin (and other coin) network. Make sure whatever we changed the protocol ID thing to isn't also used by one of the other altcoins (this can happen by random chance, so we should check).
  • Tor
    • Onion / regular partittioning
      • All traffic was going through including cleartext nodes
      • Trusted tor hidden services
        • hardcoed list of hidden services
      • cleartext<-> tor bridge nodes
  • Simulating e.g. 10 years in a week
    • Use real-world BTC data
      • Should all fail up to a certain height
  • Backup wallet (non-protocol things)
  • Task: go over all RPC commands and upgrade (i.e. insure they meet Bitcoin users' expectations and the documented behavior OR disable them).
  • Find existing tools that use the RPC commands and use them (integration test).
@nathan-at-least

This comment has been minimized.

Contributor

nathan-at-least commented Sep 12, 2016

I've put this in Beta-2, although I'm wary of not having a clear criterion for closing this ticket. I think we will need a timeout where we simply document this brainstorm. :-(

@nathan-at-least nathan-at-least changed the title from Audit the code for "Missing Required Modifications" to Document potential "Missing Required Modifications" Sep 13, 2016

@nathan-at-least

This comment has been minimized.

Contributor

nathan-at-least commented Oct 17, 2016

Let's close this after linking to it somewhere security-conscious users might notice.

@nathan-at-least nathan-at-least modified the milestones: 1.0.0-rc2, 1.0.0-rc1 Oct 17, 2016

@daira daira modified the milestones: 1.0.0-rc3, 1.0.0-rc2 Oct 23, 2016

str4d added a commit to str4d/zcash that referenced this issue Oct 25, 2016

@str4d str4d added the has PR label Oct 25, 2016

@daira

This comment has been minimized.

Contributor

daira commented Oct 25, 2016

Will be closed by #1644.

@daira daira closed this Oct 25, 2016

zkbot pushed a commit that referenced this issue Oct 25, 2016

zkbot
Auto merge of #1644 - str4d:826-reference-in-security-warnings, r=daira
Link to #826 in doc/security-warnings.md, link to new Security website page

Closes #826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment