doc: add safety.md in docs #23999

pull ghost wants to merge 8 commits into bitcoin:master from changing 1 files +62 −0
  1. ghost commented at 8:38 AM on January 7, 2022: none

    What?

    Add a new file in /doc directory with security recommendations, known issues and related things. Add 'security' section in developer notes.

    Why?

    1. Other sources of information are wiki, stackexchange, reddit, twitter etc. with their own problems
    2. There are lot of things related to security which are not documented anywhere or not organized in a way that can be easy to refer

    It was even discussed in meeting and most of the devs agreed to create a doc with security related information for Bitcoin Core: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-01-06#761399;

    Example of a similar document: https://github.com/lightningnetwork/lnd/blob/master/docs/safety.md

    How?

    I have created a doc which has 2 main sections: Users and Developers. 'Users' section is further categorized with p2p, rpc, wallet and other. 'Developers' section has two sections, one for secure programming in C++ and other for finding bugs in open pull requests.

    I have created a doc which categories p2p, rpc, wallet and other. 'Security' section in developer-notes has two things, one for secure programming in C++ and other for finding bugs in open pull requests.

    Few links that could help in understanding my perspective to add security section in developer notes:

    https://www.securecoding.com/blog/finding-and-fixing-c-vulnerabilities/

    https://bitcoincore.reviews/17639

    https://youtu.be/i0m0FBD-McY

    https://youtu.be/gHGMDFm2MVs

  2. fanquake added the label Docs on Jan 7, 2022
  3. in doc/secure.md:70 in 889b4778ba outdated 
      65 | +
  66 | +ℹ️ It’s a myth that those break cannot build.
  67 | +
  68 | +- [Developer Notes](/developer-notes.md)
  69 | +
  70 | +### Secure Programming Practices in C++
    Sjors commented at 10:45 AM on January 7, 2022:

    I would move this to a dedicated section in the developer notes and link straight to that. It prevents duplication and also makes this document less intimidating for users.

    unknown commented at 3:00 PM on January 7, 2022:

    Moved to developer notes in https://github.com/bitcoin/bitcoin/commit/05511d08be7e306a60c758ee2a64edc0cd63478e

  4. in doc/secure.md:80 in 889b4778ba outdated 
      75 | +
  76 | +**MemorySanitizer** (MSan) is a tool that detects uninitialized reads. It consists of a compiler instrumentation module and a run-time library. Typical slowdown is 3x and memory usage is 2x. MSan is supported on Linux/NetBSD/FreeBSD and requires compiling and linking all the code in your program, including libraries it uses and the C++ standard library, with -fsanitize=memory -fPIE -pie.
  77 | +
  78 | +**Valgrind** is a debugging and profiling tool suite to make programs faster and more correct. Its most popular tool, Memcheck, can detect memory-related errors common in C and C++ programs that can lead to crashes and unpredictable behavior. Here is a tutorial. Memcheck is not perfect: typical slowdown is 3-10x, memory usage is 2x, it can produce false positives (there are mechanisms for suppressing these; see the valgrind.supp file in Bitcoin Core), and it doesn’t currently detect out-of-range reads or writes to arrays allocated statically or on the stack.
  79 | +
  80 | +### Finding vulnerabilities in pull requests during reviews
    Sjors commented at 10:48 AM on January 7, 2022:

    The developer notes contain some remarks for reviewers. Maybe it makes sense to have a separate section there too.

  5. michaelfolkson commented at 11:14 AM on January 7, 2022: none

    Agree with Sjors on adding any developer related content to the developer notes and keeping this doc exclusively focused on users.

    Plus a bit nitty but I don't like the filename secure.md. Makes it sounds like we are claiming Core is definitely secure if you follow these instructions (nothing is definitely secure). Perhaps use the lnd filename safety.md if you want to avoid a name clash with already existing security.md?

  6. ghost commented at 3:02 PM on January 7, 2022: none

    Made changes based on reviews by @Sjors and @michaelfolkson

    • Rename secure.md to safety.md
    • Move developers section in developer notes
    • Fixed few links
  7. Add safety.md in docs and security section in developer notes a12fda3810
  8. unknown renamed this:
    doc: add secure.md
    doc: add safety.md in docs and security section in developer notes
    on Jan 7, 2022
  9. in doc/developer-notes.md:924 in a12fda3810 outdated 
     919 | +
 920 | +**MemorySanitizer** (MSan) is a tool that detects uninitialized reads. It consists of a compiler instrumentation module and a run-time library. Typical slowdown is 3x and memory usage is 2x. MSan is supported on Linux/NetBSD/FreeBSD and requires compiling and linking all the code in your program, including libraries it uses and the C++ standard library, with -fsanitize=memory -fPIE -pie.
 921 | +
 922 | +**Valgrind** is a debugging and profiling tool suite to make programs faster and more correct. Its most popular tool, Memcheck, can detect memory-related errors common in C and C++ programs that can lead to crashes and unpredictable behavior. Here is a tutorial. Memcheck is not perfect: typical slowdown is 3-10x, memory usage is 2x, it can produce false positives (there are mechanisms for suppressing these; see the valgrind.supp file in Bitcoin Core), and it doesn’t currently detect out-of-range reads or writes to arrays allocated statically or on the stack.
 923 | +
 924 | +### Finding vulnerabilities in pull requests during reviews
    fanquake commented at 3:02 AM on January 8, 2022:

    Finding vulnerabilities in pull requests during reviews

    ~0 on this section. There's not really anything here specific to finding "vulnerabilities", as opposed to just generally doing code review.

    In isolation, I don't think a point like, Fuzzing is a good way to find bugs., is adding much value.

    unknown commented at 4:31 AM on January 9, 2022:

    Code review can be done for lot of things. As this section is about security, finding vulnerabilities in open pull requests is one of the section that makes sense.

    Fuzzing sentence can be rephrased or add more information.

    unknown commented at 10:52 PM on January 9, 2022:

    Fuzzing is rephrased in https://github.com/bitcoin/bitcoin/pull/23999/commits/3c06f59b87116eb47c534ddb147446c4d531854c Let me know if it looks good

    jonatack commented at 9:57 AM on January 11, 2022:

    I don't think the feedback was resolved.

    -0 on these developer notes additions. They don't seem specific to Bitcoin Core. Some of these points are somewhat self-evident and they are covered much better in outside resources. As it is, this is a generic list of "things to think about while reviewing" that could encompass many things like "is this change a good idea", "does it change behavior", UB, DoS vectors, privacy leaks, locks/data races, memory efficiency, cost of attacks, serialization forward/backward compatibility, etc.

    unknown commented at 7:11 AM on January 12, 2022:

    Removed everything from developer notes that was added in this PR: 5295795b4feaeb75287fd300cc52a519e1f953da

  10. in doc/developer-notes.md:914 in a12fda3810 outdated 
     907 | @@ -907,6 +908,27 @@ Scripts
 908 |  #!/bin/bash
 909 |  ```
 910 |  
 911 | +Security
 912 | +--------------------------
 913 | +
 914 | +### Secure Programming Practices in C++
    fanquake commented at 3:16 AM on January 8, 2022:

    Secure Programming Practices in C++

    What is the purpose of this section? Some of this is basically copy-pasted from other places, i.e https://clang.llvm.org/docs/MemorySanitizer.html, in which case we'd be better off just linking to the external documentation rather than reproducing it here.

    Note that we also already have a section on Sanitizers, and our usage of them in this document, which links to many external resources.

    unknown commented at 4:34 AM on January 9, 2022:

    What is the purpose of this section?

    To add some of the things shared in:

    https://www.securecoding.com/blog/finding-and-fixing-c-vulnerabilities/

    https://youtu.be/i0m0FBD-McY

    https://youtu.be/gHGMDFm2MVs

    Some of this is basically copy-pasted from other places

    Right. This was done to not leave the section empty when drafting the pull request as I was not sure which examples of code should be mentioned here.

    unknown commented at 10:51 PM on January 9, 2022:

    Added one example in https://github.com/bitcoin/bitcoin/pull/23999/commits/3c06f59b87116eb47c534ddb147446c4d531854c

    You could suggest more in future commits

    unknown commented at 7:11 AM on January 12, 2022:

    Removed everything from developer notes that was added in this PR: 5295795b4feaeb75287fd300cc52a519e1f953da

  11. in doc/safety.md:5 in a12fda3810 outdated 
       0 | @@ -0,0 +1,58 @@
   1 | +# SECURITY
   2 | +
   3 | +This document describes the security recommendations and issues that can be helpful for users of Bitcoin Core.
   4 | +
   5 | +ℹ️ There is no patch for human stupidity.
    fanquake commented at 3:17 AM on January 8, 2022:
  12. in doc/safety.md:7 in a12fda3810 outdated 
       0 | @@ -0,0 +1,58 @@
   1 | +# SECURITY
   2 | +
   3 | +This document describes the security recommendations and issues that can be helpful for users of Bitcoin Core.
   4 | +
   5 | +ℹ️ There is no patch for human stupidity.
   6 | +
   7 | +Download Bitcoin Core only from below links, verify the integrity of the download before using:
    fanquake commented at 3:21 AM on January 8, 2022:

    Telling users to verify the integrity of the download isn't useful if you don't explain what that means, or how it should be done.

    unknown commented at 4:35 AM on January 9, 2022:

    Agree. I will add more information here in next update.

    unknown commented at 7:11 AM on January 12, 2022:

    Added one line in 088d99b5e5c9bfd907861ce68692e638ee56448d

  13. in doc/safety.md:27 in a12fda3810 outdated 
      22 | +
  23 | +Having multiple connections to the network is fundamental to safety against eclipse attacks. If a user configured the node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.
  24 | +
  25 | +`assumevalid=1` skips validation for scripts. All scripts in transactions included in blocks that are ancestors of the _assumevalid_ block are assumed to be valid. While this does introduce a little bit of trust, people can independently check that block is part of the main chain.
  26 | +
  27 | +Known incidents when BGP hijacking was used to exploit Bitcoin users: https://www.wired.com/2014/08/isp-bitcoin-theft/
    fanquake commented at 3:24 AM on January 8, 2022:

    It's a bit contradictory that in the PR description you state Other sources of information are wiki, stackexchange, reddit, twitter etc. with their own problems, then proceed to link to stackexchange, medium, wired etc throughout this document.

    wpeckr commented at 6:57 PM on January 8, 2022:

    This is really about BGP hijacks against Stratum miners, with only passing relevance to Bitcoin.

    unknown commented at 4:36 AM on January 9, 2022:

    There is some misunderstanding about the context of other sources. There are some helpful posts on all of them however it's not organised, some of it is outdated and not maintained, contains misleading information in some posts etc.

    unknown commented at 5:01 AM on January 9, 2022:

    BGP hijacking is a known issue and I could not find better examples while drafting pull request related to bitcoin.

    michaelfolkson commented at 2:51 PM on January 9, 2022:

    I think @fanquake's point is you should avoid links in this doc. Bring in/summarize the information that is relevant/useful/important from those links to this doc and don't include the links. The motivation for this doc is you don't want to refer people to these links for various reasons (e.g. mutability) but then include them in this doc which needs to be maintained (assuming it is merged). Personally I don't mind so much StackExchange links but Twitter, Reddit, Wired links would be a definite no from me.

    unknown commented at 7:12 AM on January 12, 2022:

    Removed most of the links and BGP line is completely removed in 088d99b5e5c9bfd907861ce68692e638ee56448d

  14. in doc/safety.md:49 in a12fda3810 outdated 
      44 | +
  45 | +[Encrypting the Wallet](/doc/managing-wallets.md#encrypting-the-wallet)
  46 | +
  47 | +[Backing Up the Wallet](/doc/managing-wallets.md#backing-up-the-wallet)
  48 | +
  49 | +Do not buy _wallet.dat_ files from anywhere. There are several websites that sell such files to newbies. You can read more about the issue here: https://github.com/bitcoin-core/gui/issues/77#issuecomment-721927922
    fanquake commented at 3:33 AM on January 8, 2022:

    I don't think links like this, which are to a comment in a thread which is about a completely different topic are necessarily very high quality.

    unknown commented at 4:37 AM on January 9, 2022:

    Can remove it and maybe add some relevant information here that was shared by achow101.

  15. in doc/safety.md:39 in a12fda3810 outdated 
      34 | +
  35 | +If you have a need to access it over an untrusted network like the internet, tunnel it through technology specifically designed for that, such as stunnel (SSL) or a VPN.
  36 | +
  37 | +[Executable, network access, authentication and string handling](/doc/json-rpc-interface.md#security)
  38 | +
  39 | +[Multi user systems](https://medium.com/@lukedashjr/cve-2018-20587-advisory-and-full-disclosure-a3105551e78b)
    fanquake commented at 4:22 AM on January 8, 2022:

    Multi user systems

    Multi user systems doesn't provide any context, and I'd rather not have us linking to personal medium accounts.

    luke-jr commented at 4:48 AM on January 9, 2022:

    A better description would be helpful, but objecting because it's on my Medium account is nonsense - it's no less appropriate than linking to Wired, Stack Exchange, or even bitcoincore.org for that matter.

    michaelfolkson commented at 3:02 PM on January 9, 2022:

    I disagree (certainly in comparison to say Wired, Twitter, Reddit etc). Bitcoin StackExchange has maintained immutable links and remained free to view for a decade. It is widely used and contributed to and maintained by contributors to this repo (Murch, sipa, achow101, meshcollider etc). I'm sympathetic to view that we should avoid all links in a doc but if we were to include any bitcoincore.org and Bitcoin StackExchange seem like obvious choices. I feel less strongly on personal websites of long term contributors but even they introduce single points of failure.

    unknown commented at 3:12 PM on January 9, 2022:

    I will write a Q&A on stackexchange for same thing and include the link

    sipa commented at 3:17 PM on January 9, 2022:

    I think it would be better to summarize the suggestions here inline.

    unknown commented at 10:48 PM on January 9, 2022:

    Done in https://github.com/bitcoin/bitcoin/pull/23999/commits/317ff4e931a07c18b91ff09632956f449c9aaa6e by adding summary and share cve.mitre.org link for more details

  16. in doc/safety.md:51 in a12fda3810 outdated 
      46 | +
  47 | +[Backing Up the Wallet](/doc/managing-wallets.md#backing-up-the-wallet)
  48 | +
  49 | +Do not buy _wallet.dat_ files from anywhere. There are several websites that sell such files to newbies. You can read more about the issue here: https://github.com/bitcoin-core/gui/issues/77#issuecomment-721927922
  50 | +
  51 | +Understanding the difference between legacy and descriptor wallets:
    fanquake commented at 4:23 AM on January 8, 2022:

    Understanding the difference between legacy and descriptor wallets:

    Without anything too specific here, this seems like it would be better suited to generic wallet type documentation.

    unknown commented at 4:38 AM on January 9, 2022:

    Understanding the difference in security of these two types of wallets is important

    luke-jr commented at 4:49 AM on January 9, 2022:

    I don't see how.

    sipa commented at 2:49 PM on January 9, 2022:

    I think the following suggestion would make more sense:

    • Be careful with RPC commands like dumpwallet, dumpprivkey, and listdescriptors with the "private" argument set to true. These commands reveal private keys, which should never be shared with third parties.
    • Be careful with commands like importdescriptor (others, @achow101?) if suggested by third parties, which can control future addresses created for incoming payments.
    unknown commented at 3:00 PM on January 9, 2022:

    Looks good. Will add this in next update.

  17. in doc/safety.md:17 in a12fda3810 outdated 
      12 | +
  13 | +Downloading Bitcoin Core from untrusted sources can result in using a [malware](https://bitcoin.stackexchange.com/a/107738/) that can steal your bitcoin and compromise your computer.
  14 | +
  15 | +### P2P
  16 | +
  17 | +Using non-default ports and non-listening nodes can improve security.
    wpeckr commented at 6:37 PM on January 8, 2022:

    This has no basis in fact whatsoever. The network strongly doesn't consider nodes that aren't using the standard port. Non-listening nodes have almost the exact same attack surface as listening ones, given they make promiscuous, frequent, anonymous connections to a peer to peer network.

    unknown commented at 4:42 AM on January 9, 2022:

    This has a basis and you can read more about it in below links:

    #23542

    #23306

    Non-listening nodes have almost the exact same attack surface as listening ones

    This is NOT true

    mzumsande commented at 12:30 PM on January 9, 2022:

    #23542 is currently in the state of an unmerged proposal. Until this change in policy is merged, included in releases, and adopted by a large number of nodes (which will take time), it would be detrimental for the nework if many existing nodes switched to non-standard ports, because other nodes would avoid making outbound connections to them. Therefore, I don't think this suggestion should be included at this point in time.

    sipa commented at 3:10 PM on January 9, 2022:

    I've unresolved this.

    As things are for now, and at least until #23542 is merged and in a release that is widely adopted on the network, using a non-default port is pretty much equivalent to disabling listening entirely. Outbound connections have a very strong bias away from such ports. That may change, but we shouldn't recommending it until the network actually has this policy adopted. @wpeckr Regarding difference between listening and non-listening: non-listening nodes aren't as susceptible to eclipse attacks (which typically require making connections to the victim). There are also privacy differences, as controlling your outbound connections, and even just making fewer connections, reduces how much an attacker can infer about transactions propagating through you, and originating from you.

    unknown commented at 3:13 PM on January 9, 2022:

    #23542 is currently in the state of an unmerged proposal.

    I like the assumption that this PR will be merged before #23542 :)

    sipa commented at 3:19 PM on January 9, 2022:

    I don't assume that. But I also don't assume that this PR will only be merged in 2 years, when #23542 may be common on the network.

    wpeckr commented at 7:07 PM on January 9, 2022:

    At this point in time changing the port to one that is non default is the worst of both worlds; you both will receive absolutely no incoming traffic from legitimate nodes, any you do receive will be malicious / modified software.

    unknown commented at 10:46 PM on January 9, 2022:

    It is removed

  18. in doc/safety.md:25 in a12fda3810 outdated 
      20 | +
  21 | +Difference in DNS seed and Seed node: https://bitcoin.stackexchange.com/a/17410/
  22 | +
  23 | +Having multiple connections to the network is fundamental to safety against eclipse attacks. If a user configured the node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.
  24 | +
  25 | +`assumevalid=1` skips validation for scripts. All scripts in transactions included in blocks that are ancestors of the _assumevalid_ block are assumed to be valid. While this does introduce a little bit of trust, people can independently check that block is part of the main chain.
    wpeckr commented at 6:39 PM on January 8, 2022:

    This is simply false, assumevalid accepts 0 or a block hash, it has never allowed anything else.

  19. in doc/safety.md:19 in a12fda3810 outdated 
      14 | +
  15 | +### P2P
  16 | +
  17 | +Using non-default ports and non-listening nodes can improve security.
  18 | +
  19 | +DNS seeds are used for bootstrapping the P2P network. If one of the domains used for default DNS seeds is hacked, this can impact nodes. A node checks 3 seeds at a time, so it would require coordination for a malicious seed to affect the network. You can use own DNS seed for bootstrapping.
    wpeckr commented at 6:48 PM on January 8, 2022:

    This bears very little resemblance to the actual security model of Bitcoin's bootstrap. Nobody has ever run their own DNS seed in order to bootstrap a synching node, the suggestion is complete nonsense.

    unknown commented at 4:56 AM on January 9, 2022:

    Nobody has ever run their own DNS seed in order to bootstrap a synching node, the suggestion is complete nonsense.

    I have not written history in this sentence. Anyone CAN run DNS seed using https://github.com/sipa/bitcoin-seeder

    wpeckr commented at 6:03 AM on January 9, 2022:

    Just because you could doesn't mean anybody has, or should. There is no reasonable person who would suggest a user run their own DNS seed, a system that requires large resources and setup cost, in order to synd their own node.

    mzumsande commented at 12:06 PM on January 9, 2022:

    It also makes no sense, because it just shifts the problem of bootstrapping to the seeder code without improving the solution. I don't think the seeder does anything else than ask other dns seeds to bootstrap itself (code). If all other DNS seeds feed you malicious IPs, you have gained nothing by running your own DNS seed as opposed to just running a node.

    unknown commented at 12:28 PM on January 9, 2022:

    There are two things shared in doc. 1. Running own DNS seed. It makes sense because you won't trust default DNS seeds. 2. Seed node can be used for bootstrapping. Difference is explained in the link.

    mzumsande commented at 2:20 PM on January 9, 2022:
    1. Running own DNS seed. It makes sense because you won't trust default DNS seeds.

    Disagree. I don't think the sentence "You can use own DNS seed for bootstrapping." is helpful, because even if I ran a DNS seed using https://github.com/sipa/bitcoin-seeder as suggested, this DNS seed would also need to bootstrap somehow to get going initially.

    As indicated by the link in my previous post, the bootstrapping mechanism implemented in the DNS seeder not only queries other existing DNS seeds similar to bitcoin core - so you still "trust default DNS seeds" - but is otherwise inferior to the one used by core (no fixed seeds as fallback, outdated hardcoded DNS seed list) - which makes sense because bitcoin-seeder is just not meant to be used for the use case you are suggesting here.

    sipa commented at 2:38 PM on January 9, 2022:

    Running your own DNS seed makes no sense, for multiple reasons:

    • Seeds are just a way to find an initial connection to the network. If you don't believe the DNS seeds hardcoded in Bitcoin Core are trustworthy enough for that, you should manually make connections to nodes you do trust (using -seednode / -addnode). You can also disable the DNS seeds (-dnsseed=0) in that case.
    • Bitcoin Core offers no functionality to using your own DNS seed.
    unknown commented at 2:55 PM on January 9, 2022:

    Disagree. I don't think the sentence "You can use own DNS seed for bootstrapping." is helpful, because even if I ran a DNS seed using https://github.com/sipa/bitcoin-seeder as suggested, this DNS seed would also need to bootstrap somehow to get going initially. @mzumsande

    Yes. Sentence can be improved and I am going to add/update a few things but did not because wanted to spend less time doing pull requests on weekend. Looks like this PR has lot of interest from reviewers so I will do it in next few hours.

    The attack explained in doc assumes most of the DNS seeds are safe but they might get compromised at some point. I am not sure about the code you shared @sipa can explain that better as some domains dont even resolve to anything in that. Example: bitseed.xf2.org However even if we assume these are used once in a lifetime it won't be an issue using 1-2 or get IPs based on your own crawlers.

    Running your own DNS seed makes no sense, for multiple reasons:

    Seeds are just a way to find an initial connection to the network. If you don't believe the DNS seeds hardcoded in Bitcoin Core are trustworthy enough for that, you should manually make connections to nodes you do trust (using -seednode / -addnode). You can also disable the DNS seeds (-dnsseed=0) in that case.

    Bitcoin Core offers no functionality to using your own DNS seed. @sipa -seednode thing is explained in the link and mentioned one word but I can rephrase it in a better way. Will add -dnsseed=0 on next update.

    Bitcoin Core does not offer functionality to use own DNS seed but there has to be one paragraph about trust assumption even if I remove the part that suggested running own DNS seed which is technically possible as there can be several ways to bootstrap.

    sipa commented at 2:57 PM on January 9, 2022:

    Yes, I definitely agree with explaining the trust assumptions around DNS seeds, and that one may want to avoid them. But "running your own DNS seed" is pointless as a solution for it.

    unknown commented at 10:46 PM on January 9, 2022:

    It is removed

  20. in doc/safety.md:29 in a12fda3810 outdated 
      24 | +
  25 | +`assumevalid=1` skips validation for scripts. All scripts in transactions included in blocks that are ancestors of the _assumevalid_ block are assumed to be valid. While this does introduce a little bit of trust, people can independently check that block is part of the main chain.
  26 | +
  27 | +Known incidents when BGP hijacking was used to exploit Bitcoin users: https://www.wired.com/2014/08/isp-bitcoin-theft/
  28 | +
  29 | +Do not use blocks and chainstate from unknown sources. Attackers could be providing you a false blockchain, i.e. one that is valid but not the blockchain that all other nodes are following.
    wpeckr commented at 6:50 PM on January 8, 2022:

    Somebody providing you a different, valid chain state isn't really any concern as you would just reorganize back to the majority chain as soon as it was visible. The issue is that the chain state isn't validated and so may just contain fictitious invalid entries. Someone giving you blocks is of no concern whatsoever and was even a suggested method previously.

    luke-jr commented at 4:46 AM on January 9, 2022:

    While that's true of someone providing you with blocks, it isn't true for the chainstate. If you use a compromised chainstate, you may never find out until you have a financial loss.

    unknown commented at 4:57 AM on January 9, 2022:

    Somebody providing you a different, valid chain state isn't really any concern Someone giving you blocks is of no concern whatsoever and was even a suggested method previously.

    This is NOT true

    sipa commented at 2:40 PM on January 9, 2022:

    Yeah, the concern is only about the chainstate, as that contains post-validated information. If you accept a chainstate from someone else, they can make your node convinced about anything (coins that don't exist/do exist, ...).

    There is no problem with accepting just blocks from someone (IF combined with a -reindex-chainstate, or just an empty chainstate; in both cases the blocks will still be validated).

    So you should never accept a chaincode from untrusted sources, but the reason isn't a worry about a valid chainstate for an incorrect chain (like @wpeckr points out), but the fact that it may be an invalid chainstate for the correct chain.

    unknown commented at 10:45 PM on January 9, 2022:

    I agree with sipa and luke-jr and could not find anything to change so its still the same thing. Also it was mostly copied from an answer on stackexchange by achow101 to avoid issues in misunderstanding.

    wpeckr commented at 4:53 AM on January 10, 2022:

    The text then remains incorrect; there's no concern about somebody giving you a valid copy of the chainstate, quite literally as it would be valid and everything is totally fine, the concern is about an invalid chainstate as this is undetectable and can lead to losses.

    unknown commented at 9:57 AM on January 10, 2022:

    https://bitcoin.stackexchange.com/a/63860/

  21. in doc/safety.md:58 in a12fda3810 outdated 
      53 | +There is no XPUB in legacy wallets, `dumpwallet` will return private keys. `listdescriptors` can be used to get XPUB and XPRV for descriptor wallets.
  54 | +
  55 | +
  56 | +### Other
  57 | +
  58 | +_*notify_ config and commandline options accept shell commands to be based on different events in Bitcoin Core. Be careful while using these options and ensure that the commands are safe either by manual inspection or restrictions/alerts in the system used.
    wpeckr commented at 6:59 PM on January 8, 2022:

    As this is the purpose for the creation of this document, I should point out that this still is a completely value-free suggestion. Almost no system could be sufficiently "locked down" to prevent those commands from being used in the way which is being claimed to be malicious, and the situations which would require a system to be restricted to this degree are completely beyond reason. The previous discussion in #23412 and #23850 is comprehensive.

    unknown commented at 5:04 AM on January 9, 2022:

    As this is the purpose for the creation of this document

    No the purpose this pull request and docs is mentioned in OP

    Almost no system could be sufficiently "locked down" to prevent those commands from being used in the way which is being claimed to be malicious

    This is NOT true.

    Some people prefer to use restrictions and alerts in systems, networks etc.

    sipa commented at 2:44 PM on January 9, 2022:

    It would be better to instead just have a generic warning to be careful about advice to configure your nodes by untrustworthy sources. If a malicious actor can convince your to modify your configuration, they can probably also convince you to open up your RPC server to them, letting them steal your money.

    unknown commented at 3:08 PM on January 9, 2022:

    Disagree with this and @wpeckr agrees that config like rpcbind should be removed or restricted in #18105 because people follow wrong things. So I don't understand this contradiction.

    There are several other things that are already shared in other issues and pull requests which I don't want to repeat.

    sipa commented at 3:15 PM on January 9, 2022:

    I'm not talking about rpcbind. That's a separate discussion to be had.

    I don't think there is anything special about the -notify* commands that warrants calling them out here. The concern is users using configuration they don't understand. That is what we should be educating about. Perhaps we can include some examples of dangerous configurations/commands (like the dump/import commands I've mentioned elsewhere, or a list of RPC commands that may affect funds (send*), ...). But I worry about giving an impression of exhaustiveness even.

    unknown commented at 3:35 PM on January 9, 2022:

    I'm not talking about rpcbind. That's a separate discussion to be had.

    Its one of the config

    I don't think there is anything special about the -notify* commands that warrants calling them out here.

    We cannot trigger scripts based on events in bitcoin core with any other command line parameter.

    sipa commented at 3:49 PM on January 9, 2022:

    We cannot trigger scripts based on events in bitcoin core with any other command line parameter.

    I'm well aware. I don't think that's particularly relevant for security, if users are willing to make arbitrary changes they don't understand.

    I'm not saying this isn't a concern, I'm saying this isn't a concern that can be addressed, if you're operating in the assumption that users will make configuration changes or run commands they don't understand.

    To use an analogy. There is an estate with a walled garden around it and cameras for security. Inside the estate is a mansion, with a terrace. The door to the terrace is often left unlocked in summer while people walk in and out. You're complaining about the fact that people leave the door unlocked, which, while true, isn't a concern because the wall and cameras are supposed to keep attackers out. If the wall and cameras didn't work, there are probably tons of other ways of getting inside the house, because the whole thing relies on it; the window is often left unlocked too.

    The wall is users knowing that making configuration changes they don't understand can be dangerous. The terrace door is the notify commands. The summer is the advanced users who have a need for those commands. The window left open is running a "sendtoaddress" command to empty their wallet.

    Yes, the notify commands are special in that they're the only ones that will make Bitcoin Core execute something else. So what? There are tons of other ways a user can be harmed if they don't understand what they're doing. We should be educating people about the risk of unknown commands; not calling these config options out specifically. Doing so might even have the opposite effect: people may think that anything but these options cannot cause harm.

    I'm not against mentioning these options as an example of something that can go wrong if used without understanding. But such a list would include many more commands and options.

    Please unresolve this.

    unknown commented at 4:01 PM on January 9, 2022:

    I'm not against mentioning these options as an example of something that can go wrong if used without understanding. But such a list would include many more commands and options.

    'Other' section was kept for such things. I mentioned one thing that I recently observed.

    Please unresolve this.

    I would unresolve this. However, *notify options are not secure as implemented in Bitcoin Core.

    sipa commented at 4:02 PM on January 9, 2022:

    I would unresolve this. However, *notify options are not secure as implemented in Bitcoin Core.

    That's your opinion. I disagree, and apparently many others do too. You're entitled to your opinion, but that doesn't mean it should be included in this document.

    unknown commented at 10:43 PM on January 9, 2022:

    I have rephrased the whole thing based on IRC conversation. Let me know if that looks good?

    wpeckr commented at 5:09 AM on January 10, 2022:

    *notify config and commandline options accept shell commands to be based on different events in Bitcoin Core. Be careful while using these options and ensure that the commands are safe either by manual inspection or restrictions/alerts in the system used.

    vs

    *notify config and commandline options accept shell commands to be based on different events in Bitcoin Core. Ensure that the commands are safe either by manual inspection or restrictions/alerts in the system used.

    It's functionally unchanged as far as I can tell. The issue it with the concept of telling people that this is a "issue" to begin with, not the wording.

    unknown commented at 10:15 AM on January 10, 2022:

    The issue it with the concept of telling people that this is a "issue" to begin with, not the wording.

    Sorry I have not used any financial applications that allow such things, with no restrictions and no documentation. This is a known issue for some people and not an issue for others. The compromise can be to rephrase things in a way that does not look misleading but helps users. I have tried my best to write same thing in different ways in different pull requests and we can continue this discussion forever without changing our opinion.

    Restricting scripts is not a weird thing because I have seen such things used in PowerShell with Set-ExecutionPolicy. Creating alerts for malware that might try to connect with attacker's domain, IP etc. is also not a bad advice and most of the people already use firewalls. Macros in word allow executing anything but even that has some restrictions or options to disable/enable.

    wpeckr commented at 6:32 PM on January 10, 2022:

    Sorry I have not used any financial applications that allow such things, with no restrictions and no documentation.

    This is a straw man, this is about Bitcoin, not other software.

    This is a known issue for some people and not an issue for others.

    This is not a known issue. As has been extensively discussed, you are the only person here who has opened multiple pull requests and issues about a completely absurd situation that no reasonable person could consider problematic. You are not a martyr, you are not fighting against the tyranny of other contributors, or whatever you think is going on here. You are simply incorrect and are being too childish to admit it.

    Macros in word allow executing anything but even that has some restrictions or options to disable/enable.

    Word is an application that allows end users to open documents from arbitrary sources, it bears absolutely no relevance to your argument.

    unknown commented at 6:51 PM on January 10, 2022:

    This is not a known issue.

    #23395 (comment)

    you are the only person here who has opened multiple pull requests and issues about a completely absurd situation that no reasonable person could consider problematic.

    There is nothing absurd or unreasonable in this. Feel free to NACK this PR if you don't agree with the changes.

    You are not a martyr, you are not fighting against the tyranny of other contributors, or whatever you think is going on here. You are simply incorrect and are being too childish to admit it.

    This is non sense.

    Word is an application that allows end users to open documents from arbitrary sources, it bears absolutely no relevance to your argument.

    It does because of the feature in context

  22. JeremyRubin commented at 7:34 PM on January 8, 2022: contributor

    concept ack for having this

  23. Remove emoji and note
    Co-authored-by: Michael Ford <fanquake@gmail.com>
    201e886d82
  24. in doc/safety.md:12 in 201e886d82 outdated 
       7 | +
   8 | +https://bitcoincore.org/en/download/
   9 | +
  10 | +https://github.com/bitcoin/bitcoin/releases
  11 | +
  12 | +Downloading Bitcoin Core from untrusted sources can result in using a [malware](https://bitcoin.stackexchange.com/a/107738/) that can steal your bitcoin and compromise your computer.
    luke-jr commented at 4:43 AM on January 9, 2022:

    Maybe explicitly mention where to find official RCs, and caution that any solicitation for private testing is likely malware (though we want to be careful not to dissuade users from heloing actually debug issues, so how it's phrased should consider that too)

    unknown commented at 5:05 AM on January 9, 2022:

    Sure. Will add more information today.

    unknown commented at 7:14 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  25. in doc/safety.md:22 in 201e886d82 outdated 
      17 | +
  18 | +DNS seeds are used for bootstrapping the P2P network. If one of the domains used for default DNS seeds is hacked, this can impact nodes. A node checks 3 seeds at a time, so it would require coordination for a malicious seed to affect the network. You can use own DNS seed for bootstrapping.
  19 | +
  20 | +Difference in DNS seed and Seed node: https://bitcoin.stackexchange.com/a/17410/
  21 | +
  22 | +Having multiple connections to the network is fundamental to safety against eclipse attacks. If a user configured the node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.
    luke-jr commented at 4:44 AM on January 9, 2022:

    Only 8 are expected, and only 8 are made normally.

    unknown commented at 5:06 AM on January 9, 2022:

    8 full relay and 2 block relay?

    sipa commented at 2:44 PM on January 9, 2022:

    Indeed, 8 full relay and 2 block relay.

    unknown commented at 7:14 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  26. in doc/safety.md:24 in 201e886d82 outdated 
      19 | +
  20 | +Difference in DNS seed and Seed node: https://bitcoin.stackexchange.com/a/17410/
  21 | +
  22 | +Having multiple connections to the network is fundamental to safety against eclipse attacks. If a user configured the node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.
  23 | +
  24 | +`assumevalid=1` skips validation for scripts. All scripts in transactions included in blocks that are ancestors of the _assumevalid_ block are assumed to be valid. While this does introduce a little bit of trust, people can independently check that block is part of the main chain.
    luke-jr commented at 4:45 AM on January 9, 2022:

    The only way to independently check the block is part of the main chain, is to run without assumevalid...

    unknown commented at 7:15 AM on January 12, 2022:

    This commit is outdated. Please suggest changes in latest commit if I could improve something in this line.

  27. luke-jr changes_requested
  28. fix assumevalid syntax 921d6da421
  29. wpeckr commented at 6:15 AM on January 9, 2022: none

    This document has serious quality issues. It makes a number of claims which do not reflect concerns which an administrator or operator of a node might realistically be able to make use of, or do not reflect standard practice that anybody in the community might be able to follow. It suggests unrealistic situations like a user running their own DNS seeder to prevent issues bootstrapping with the network, changing the p2p port number in a fictitious attempt to increase their security, and that there is somehow a vulnerability in -blocknotify which needs resolution with external tools and configuration.

  30. ghost commented at 6:26 AM on January 9, 2022: none

    This document has serious quality issues. It makes a number of claims which do not reflect concerns which an administrator or operator of a node might realistically be able to make use of, or do not reflect standard practice that anybody in the community might be able to follow. It suggests unrealistic situations like a user running their own DNS seeder to prevent issues bootstrapping with the network, changing the p2p port number in a fictitious attempt to increase their security, and that there is somehow a vulnerability in -blocknotify which needs resolution with external tools and configuration.

    This is NOT true. Thanks for sharing your opinion anyway.

  31. mzumsande commented at 12:19 PM on January 9, 2022: contributor

    I don't think that most the points by @wpeckr above are resolved. If there is a difference in opinion, it is particularly important to leave these threads open for a while so that others can weigh in and agree with either side or add their own views. Please unresolve those threads.

  32. ghost commented at 12:35 PM on January 9, 2022: none

    I don't think that most the points by @wpeckr above are resolved. If there is a difference in opinion, it is particularly important to leave these threads open for a while so that others can weigh in and agree with either side or add their own views. Please unresolve those threads. @mzumsande everything has been answered and resolved based on my understanding. If you have issues with something feel free to comment.

    I have unresolved one comment about DNS seed in which writing "non sense" does not help in improving the document. This is a pull request in draft mode and work in progress which is even mentioned in OP.

    Also I can not change/remove everything mentioned by @wpeckr who has written lot of irrelevant/false things. Things that make sense have already been changed. Example: assumevalid syntax

  33. in doc/safety.md:32 in 921d6da421 outdated 
      27 | +
  28 | +Do not use blocks and chainstate from unknown sources. Attackers could be providing you a false blockchain, i.e. one that is valid but not the blockchain that all other nodes are following.
  29 | +
  30 | +### RPC
  31 | +
  32 | +The JSON-RPC interface is intended for local access. By default it will only accept connections from localhost, but it can be configured to be accessed for a wider netmask; e.g. a trusted LAN network. Bitcoin Core does not allow anyone to connect to the rpcport. You will need to explicitly allow an IP address to connect to it by using the `rpcallowip=<ip>` option in the _bitcoin.conf_ file. If you set it to 0.0.0.0, it will be open to all IP addresses, but this is not recommended as it is not secure.
    sipa commented at 2:46 PM on January 9, 2022:

    I don't understand "Bitcoin Core does not allow anyone to connect to the rpcport". Clearly it can be configured to, or RPC would be useless?

    unknown commented at 2:58 PM on January 9, 2022:

    It can be configured to but does not allow by default. It was copied from this answer: https://bitcoin.stackexchange.com/a/69097/

    sipa commented at 3:01 PM on January 9, 2022:

    Ok, then say that? The phrase as is is very confusing.

  34. michaelfolkson commented at 3:20 PM on January 9, 2022: none

    I don't think that most the points by @wpeckr above are resolved. Please unresolve those threads.

    This document has serious quality issues. @prayank23: If you aren't going to listen, attempt to resolve concerns people have and instead bat them away with immature responses like "This is NOT true. Thanks for sharing your opinion anyway" then I will Approach NACK this.

    I said on IRC:

    19:23 <michaelfolkson> As long as things that are argued over/disputed are stripped out seems like a good idea to me too

    Documents in this repo should avoid being opinionative or disputed as they have to be maintained and defended in perpetuity. If you can't respect that then I recommend you close this and leave it to someone else to pick it up.

  35. ghost commented at 3:27 PM on January 9, 2022: none

    @michaelfolkson

    @prayank23: If you aren't going to listen

    Thanks for your moderation on GitHub. You cannot force changes in a pull request based on one/two suggestions. Yes some of them are NOT TRUE. If you don't agree with the changes that are still in draft mode anyway feel free to NACK.

    I said on IRC:

    And I mentioned even if the PR doesn't get merged I would know about the issues that should be documented. So please don't threaten me with NACKs because changes were not made on weekend based on some comments by one reviewer.

  36. sipa commented at 3:33 PM on January 9, 2022: member

    @prayank23 I Concept ACK having a document like this, and you've been getting a lot of feedback on it. If addressed, I think the result could be very usable.

    That said, I think you shouldn't just mark things resolved just because you disagree with them. Not all comments are correct, but many are at least right in pointing out controversies in the text here, and those ought to be addressed before a document like this is merged. You're of course free to decide not to address any particular comment by marking it resolved, but you should accept that in that case people may disagree with merging it.

    An alternative is responding "I'm not sure about this, let's see what others think", and not addressing it for the time being, but also not marking it resolved.

  37. DrahtBot commented at 8:35 PM on January 9, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  38. add example for Integer wraps and rephrase fuzzing 3c06f59b87
  39. rephrase dns seeds, add more info for multi user, wallet and *notify 317ff4e931
  40. ghost commented at 10:12 PM on January 9, 2022: none

    Made few changes in PR based on comments that made sense. It is still in draft mode. Most of the comments are addressed. Others will be addressed in next update and comment accordingly.

  41. Fix RPC info 8a94f98901
  42. namcios commented at 4:45 PM on January 10, 2022: none

    Concept ACK

  43. wpeckr commented at 6:50 PM on January 10, 2022: none

    It doesn't seem of value for me to comment on this further, however this remains low quality and doesn't convey much in the way of useful information that anybody could take action on or take into account for their threat modelling. Some of the "resolved" issues are just marked as such to hide the discussion about flaws in the text, which is of note to future reviewers.

  44. Remove security section from dev notes 5295795b4f
  45. 1337in commented at 6:55 AM on January 11, 2022: none

    Concept ACK

  46. in doc/developer-notes.md:924 in 8a94f98901 outdated 
     919 | +  - Postfix expressions just before square brackets [ ]
 920 | +  - Functional arguments to a memory allocation function
 921 | +  - Assignment expressions used to declare a variable-length array
 922 | +  - Code that is critical to security
 923 | +
 924 | +An integer wrap can occur when multiplying two operands resulting in insufficient memory size allocation, as shown in the code snippet below. Here, the signed int value is converted to size t before the multiplication operation. This means multiplication will occur between two unsigned size t integers
    jonatack commented at 9:51 AM on January 11, 2022:

    I'm not really sure about adding this section, but:

    An integer wrap can occur when multiplying two operands, resulting in insufficient memory size allocation, as shown in the code snippet below. Here, the signed int value is converted to `size_t` before the multiplication operation. This means multiplication will occur between two unsigned `size_t` integers.
    unknown commented at 7:16 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  47. in doc/developer-notes.md:956 in 8a94f98901 outdated 
     951 | +
 952 | +1. Do not trust the PR author based on things mentioned in the PR description. Review and verify everything.
 953 | +
 954 | +2. Test manually based on things that may go wrong and not tried in functional tests.
 955 | +
 956 | +3. [Fuzzing](/fuzzing.md) helps to find bugs in the code and doucmented in detail. You can also try fuzzing using third party software like burpsuite that is used by lot of penetration testers. It will work for RPC and you could write your scripts as well for fuzzing in any language if testing RPC using POST requests.
    jonatack commented at 10:00 AM on January 11, 2022:

    No need to replace fuzzing.md here IMO, but

    3. [Fuzzing](/fuzzing.md) helps to find bugs in the code and documented in detail. You can also try fuzzing using third party software like burpsuite that is used by lot of penetration testers. It will work for RPC and you could write your scripts as well for fuzzing in any language if testing RPC using POST requests.
    unknown commented at 7:16 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  48. in doc/safety.md:6 in 8a94f98901 outdated 
       0 | @@ -0,0 +1,61 @@
   1 | +# SECURITY
   2 | +
   3 | +This document describes the security recommendations and issues that can be helpful for users of Bitcoin Core.
   4 | +
   5 | +
   6 | +Download Bitcoin Core only from below links, verify the integrity of the download before using:
    jonatack commented at 10:01 AM on January 11, 2022: 
    Download Bitcoin Core only from the links listed below and verify the integrity of the download before using:
    unknown commented at 7:16 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  49. in doc/safety.md:16 in 8a94f98901 outdated 
      11 | +
  12 | +Downloading Bitcoin Core from untrusted sources can result in using a [malware](https://bitcoin.stackexchange.com/a/107738/) that can steal your bitcoin and compromise your computer.
  13 | +
  14 | +### P2P
  15 | +
  16 | +DNS seeds are used for bootstrapping the P2P network. If one of the domains used for default DNS seeds is hacked, this can impact nodes. A node checks 3 seeds at a time, so it would require coordination for a malicious seed to affect the network. If you don't believe the DNS seeds hardcoded in Bitcoin Core, you should manually make connections to nodes you do trust (using `-seednode` / `-addnode`). You can also disable the DNS seeds (`-dnsseed=0`) in that case.
    jonatack commented at 10:02 AM on January 11, 2022: 
    DNS seeds are used for bootstrapping the P2P network. If one of the domains used for default DNS seeds is hacked, this can impact nodes. A node checks 3 seeds at a time, so it would require coordination for a malicious seed to affect the network. If you prefer not to trust the DNS seeds hardcoded in Bitcoin Core, you should manually make connections to nodes you do trust (using `-seednode` / `-addnode`). You can also disable the DNS seeds (`-dnsseed=0`) in that case.
    unknown commented at 7:17 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  50. in doc/safety.md:36 in 8a94f98901 outdated 
      31 | +
  32 | +If you have a need to access it over an untrusted network like the internet, tunnel it through technology specifically designed for that, such as stunnel (SSL) or a VPN.
  33 | +
  34 | +[Executable, network access, authentication and string handling](/doc/json-rpc-interface.md#security)
  35 | +
  36 | +CVE-2018–20587 is an Incorrect Access Control vulnerability that affects all currently released versions of Bitcoin Core, you may be affected by this issue if you have the RPC service enabled (default, with the headless bitcoind), and other users have access to run their own services on the same computer (either local or remote access). If you are the only user of the computer running your node, you are not affected. If you use the GUI but have not enabled the RPC service, you are not affected.
    jonatack commented at 10:05 AM on January 11, 2022: 
    CVE-2018–20587 is an Incorrect Access Control vulnerability that affects all currently released versions of Bitcoin Core. You may be affected by this issue if you have the RPC service enabled (default, with the headless bitcoind) and other users have access to run their own services on the same computer (either local or remote access). If you are the only user of the computer running your node, you are not affected. If you use the GUI but have not enabled the RPC service, you are not affected.
    unknown commented at 7:17 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  51. in doc/safety.md:20 in 8a94f98901 outdated 
      15 | +
  16 | +DNS seeds are used for bootstrapping the P2P network. If one of the domains used for default DNS seeds is hacked, this can impact nodes. A node checks 3 seeds at a time, so it would require coordination for a malicious seed to affect the network. If you don't believe the DNS seeds hardcoded in Bitcoin Core, you should manually make connections to nodes you do trust (using `-seednode` / `-addnode`). You can also disable the DNS seeds (`-dnsseed=0`) in that case.
  17 | +
  18 | +Difference in DNS seed and Seed node: https://bitcoin.stackexchange.com/a/17410/
  19 | +
  20 | +Having multiple connections to the network is fundamental to safety against eclipse attacks. If a user configured the node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.
    jonatack commented at 10:07 AM on January 11, 2022:

    Suggest using either the second person ("you") or third person ("a user") consistently throughout the document.

    Having multiple connections to the network is fundamental to safety against eclipse attacks. If you configure your node to only have outbound block-relay-only connections, they wouldn't be participating in addr relay, so wouldn't have a diverse addrman to identify candidates to connect to. It is suggested to have at least 10 outbound connections.

    Also, try for consistent verb tense use throughout the document--either present or past tense, for example. I'd suggest present tense.

    unknown commented at 7:17 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  52. in doc/safety.md:54 in 8a94f98901 outdated 
      49 | +
  50 | +Understanding the difference between legacy and descriptor wallets:
  51 | +
  52 | +Be careful with RPC commands like `dumpwallet`, `dumpprivkey`, and `listdescriptors` with the "private" argument set to true. These commands reveal private keys, which should never be shared with third parties.
  53 | +
  54 | +Be careful with commands like `importdescriptor` if suggested by third parties, which can control future addresses created for incoming payments
    jonatack commented at 10:09 AM on January 11, 2022: 
    Be careful with commands like `importdescriptor` if suggested by third parties, which can control future addresses created for incoming payments.
    unknown commented at 7:17 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  53. in doc/safety.md:61 in 8a94f98901 outdated 
      56 | +
  57 | +### Other
  58 | +
  59 | +Be careful about accepting configuration changes and commands from third parties. Here is a non-exhaustive list of things that may go wrong:
  60 | +
  61 | +_*notify_ config and commandline options accept shell commands to be based on different events in Bitcoin Core. Ensure that the commands are safe either by manual inspection or restrictions/alerts in the system used.
    jonatack commented at 10:09 AM on January 11, 2022: 
    _*notify_ config and command line options accept shell commands to be based on different events in Bitcoin Core. Ensure that the commands are safe either by manual inspection or restrictions/alerts in the system used.
    unknown commented at 7:17 AM on January 12, 2022:

    Done in 088d99b5e5c9bfd907861ce68692e638ee56448d

  54. jonatack commented at 10:15 AM on January 11, 2022: member

    Concept ACK on safety.md but I think you are trying to do too much here; would suggest making it pithier by honing it down to the most useful/pertinent points. Reducing scope and length may make it easier to garner review consensus.

    -0 on most or all of the developer notes additions (or propose them separately) as (a) they don't seem specific to Bitcoin Core and (b) some of these points are somewhat self-evident and they are covered much better in outside resources. Some of it is only a generic list of "things to think about while reviewing" that could be somewhat limitless, encompassing items like "is this change a good idea", "does it change behavior", UB, DoS vectors, privacy leaks, locks/data races, memory efficiency, cost of attacks, serialization forward/backward compatibility, etc.

    Left a few grammar suggestion.

  55. Update doc based on feedback from reviewers 088d99b5e5
  56. ghost commented at 7:27 AM on January 12, 2022: none

    Thanks @jonatack Agree, I have made lot of changes in last 2 commits and addressed most of the comments.

    This is a summary of all the things that have been changed in this pull request since it was opened based on feedback from reviewers: @Sjors

    Initially security for developers and users was in the same doc. Separated the developers section which is completely removed after last commit @fanquake

    • Rename secure.md to safety.md @wpeckr
    • Remove line about using non-default ports
    • Fix assumevalid syntax
    • Remove suggestion to run DNS seeder
    • Remove BGP hijacking line and link to article. It is still an issue that could affect security and not just limited to Bitcoin. There is nothing much that users would achieve reading the link, nothing better was suggested in review so removed it. @sipa
    • Add information about legacy and descriptor wallets
    • Remove suggestion to run DNS seeder
    • Remove line about using non-default ports
    • Remove one line in RPC info to avoid misunderstanding
    • Add (-seednode / -addnode) and (-dnsseed=0)
    • Rephrase warning about *notify based on suggestion discussed in IRC @mzumsande
    • Remove suggestion to run DNS seeder
    • Remove line about using non-default ports @luke-jr
    • Mention about solicitation for private testing can be a malware
    • Mention 8 full relay and 2 block only in bracket @jonatack
    • Fix grammar issues
    • Remove security section from developer notes

    ⚠️ Only 1 thing that could still be improved and I am not sure about:

    • Line about blocks and chainstate

    Related discussion: #23999 (review)

    If I missed anything else apology as it was not easy going through more than 100 comments in the pull request. I have resolved things that are addressed based on my understanding. It will be impossible for anyone to review this PR if every thread is kept open as things look confusing. Nothing is hidden, no commits are squashed. Also there is always room for improvement.

    Thanks everyone for reviewing the pull request. I am marking this finally as 'ready for review'.

  57. unknown marked this as ready for review on Jan 12, 2022
  58. in doc/safety.md:49 in 088d99b5e5 
      44 | +
  45 | +[Encrypting the Wallet](/doc/managing-wallets.md#encrypting-the-wallet)
  46 | +
  47 | +[Backing Up the Wallet](/doc/managing-wallets.md#backing-up-the-wallet)
  48 | +
  49 | +Do not buy _wallet.dat_ files from anywhere. There are several websites that sell such files to newbies. Scammers modify _wallet.dat_ file to include a bogus ckey record.
    luke-jr commented at 7:39 AM on January 12, 2022:

    Maybe generalise this to not using untrusted wallet files?

  59. unknown renamed this:
    doc: add safety.md in docs and security section in developer notes
    doc: add safety.md in docs
    on Jan 12, 2022
  60. bitcoin locked this on Apr 16, 2025
Contributors
ghostSjorsmichaelfolksonfanquakejonatackwpeckrluke-jrsipamzumsandeJeremyRubinDrahtBotnamcios1337in
Labels
Docs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me