randomenv: remove some /proc/ accesses #32450

1. 
   fanquake commented at 5:20 pm on May 8, 2025: member

   Any env data we try and gather is on a best-effort basis, however in this instance, for multiple users, it’s just producing spam. This could probably be solved with further snap/apparmor configuration, however given that no-one is actively working on snap packaging, we could instead drop this source of env data.

   Closes https://github.com/bitcoin-core/packaging/issues/115. Also in https://github.com/bitcoin-core/packaging/issues/217.

   Happy to close this if someone opens an alternative PR (in https://github.com/bitcoin-core/packaging) fixing the Snap AppArmor configuration.

2. randomenv: remove some /proc/ accesses

   Any env data we try and gather is on a best-effort basis, however
   in this instance, for multiple users, it's just producing spam.
   This could probably be solved with further snap/apparmor
   configuration, however given that no-one is actively working on
   snap packaging, we could instead drop this source of env data.
   
   Closes https://github.com/bitcoin-core/packaging/issues/115.
   Also in https://github.com/bitcoin-core/packaging/issues/217.

   64a0d6f995
3. 
   DrahtBot commented at 5:21 pm on May 8, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32450.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

4. 
   laanwj commented at 5:42 pm on May 8, 2025: member

   As i have some experience setting up apparmor i looked into this for a bit, but sadly it’s not simply a matter of adding some apparmor rules. Snap works with larger sets of permissions like system-observe process-control. We likely want to avoid granting these to bitcoind just to collect random data.

   An alternative to dropping these completely would be to try once, and if the access fails, don’t try again.

   E.g. simplest implementation: pass a reference to a static flag to every AddFile( and set it to false after a failure. If it’s false at start, don’t try again.

5. laanwj added the label Utils/log/libs on May 8, 2025
6. 
   laanwj commented at 12:08 pm on May 9, 2025: member

   E.g. simplest implementation: pass a reference to a static flag to every AddFile( and set it to false after a failure. If it’s false at start, don’t try again.

   Alternative using command pattern:

     0diff --git a/src/randomenv.cpp b/src/randomenv.cpp
     1index fbab23afe94315faa65d58d15ee0856875e1e92c..411985c662ea904eeaa983ed87e7597dd1485509 100644
     2--- a/src/randomenv.cpp
     3+++ b/src/randomenv.cpp
     4@@ -94,10 +94,29 @@ void AddSockaddr(CSHA512& hasher, const struct sockaddr *addr)
     5     }
     6 }
     7 
     8-void AddFile(CSHA512& hasher, const char *path)
     9+/**
    10+ * Add the contents and stat metadata of a file to the random environment,
    11+ * on a best-effort basis.
    12+ *
    13+ * To avoid audit logging spam, this keeps track of an error flag.
    14+ * This will be set on error, and the action will not be retried next time.
    15+ */
    16+class FileAdder {
    17+public:
    18+    FileAdder(std::string_view path):
    19+        m_path(path) {}
    20+
    21+    void Run(CSHA512& hasher);
    22+private:
    23+    const std::string m_path;
    24+    bool m_error_flag{false};
    25+};
    26+
    27+void FileAdder::Run(CSHA512& hasher)
    28 {
    29+    if (m_error_flag) return;
    30     struct stat sb = {};
    31-    int f = open(path, O_RDONLY);
    32+    int f = open(m_path.c_str(), O_RDONLY);
    33     size_t total = 0;
    34     if (f != -1) {
    35         unsigned char fbuf[4096];
    36@@ -111,6 +130,8 @@ void AddFile(CSHA512& hasher, const char *path)
    37             /* not bothering with EINTR handling. */
    38         } while (n == sizeof(fbuf) && total < 1048576); // Read only the first 1 Mbyte
    39         close(f);
    40+    } else {
    41+        m_error_flag = true;
    42     }
    43 }
    44 
    45@@ -223,15 +244,20 @@ void RandAddDynamicEnv(CSHA512& hasher)
    46 #endif
    47 
    48 #ifdef __linux__
    49-    AddFile(hasher, "/proc/diskstats");
    50-    AddFile(hasher, "/proc/vmstat");
    51-    AddFile(hasher, "/proc/schedstat");
    52-    AddFile(hasher, "/proc/zoneinfo");
    53-    AddFile(hasher, "/proc/meminfo");
    54-    AddFile(hasher, "/proc/softirqs");
    55-    AddFile(hasher, "/proc/stat");
    56-    AddFile(hasher, "/proc/self/schedstat");
    57-    AddFile(hasher, "/proc/self/status");
    58+    static std::vector<FileAdder> file_adders = {
    59+        {"/proc/diskstats"},
    60+        {"/proc/vmstat"},
    61+        {"/proc/schedstat"},
    62+        {"/proc/zoneinfo"},
    63+        {"/proc/meminfo"},
    64+        {"/proc/softirqs"},
    65+        {"/proc/stat"},
    66+        {"/proc/self/schedstat"},
    67+        {"/proc/self/status"}
    68+    };
    69+    for (auto &adder : file_adders) {
    70+        adder.Run(hasher);
    71+    }
    72 #endif
    73 
    74 #ifdef HAVE_SYSCTL
    75@@ -365,16 +391,16 @@ void RandAddStaticEnv(CSHA512& hasher)
    76     AddPath(hasher, "/home");
    77     AddPath(hasher, "/proc");
    78 #ifdef __linux__
    79-    AddFile(hasher, "/proc/cmdline");
    80-    AddFile(hasher, "/proc/cpuinfo");
    81-    AddFile(hasher, "/proc/version");
    82+    FileAdder("/proc/cmdline").Run(hasher);
    83+    FileAdder("/proc/cpuinfo").Run(hasher);
    84+    FileAdder("/proc/version").Run(hasher);
    85 #endif
    86-    AddFile(hasher, "/etc/passwd");
    87-    AddFile(hasher, "/etc/group");
    88-    AddFile(hasher, "/etc/hosts");
    89-    AddFile(hasher, "/etc/resolv.conf");
    90-    AddFile(hasher, "/etc/timezone");
    91-    AddFile(hasher, "/etc/localtime");
    92+    FileAdder("/etc/passwd").Run(hasher);
    93+    FileAdder("/etc/group").Run(hasher);
    94+    FileAdder("/etc/hosts").Run(hasher);
    95+    FileAdder("/etc/resolv.conf").Run(hasher);
    96+    FileAdder("/etc/timezone").Run(hasher);
    97+    FileAdder("/etc/localtime").Run(hasher);
    98 #endif
    99 
   100     // For MacOS/BSDs, gather data through sysctl instead of /proc. Not all of these
   

   This principle could potentially be extended to the other operations, if they need similar precautions.

Contributors
fanquake DrahtBot laanwj

Labels
Utils/log/libs