Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#1168 closed Defect (fixed)

Very stupid bug on FILE_INFO::verify_file_certs()

Reported by: matszpk Owned by:
Priority: Critical Milestone: Undetermined
Component: Undetermined Version: 6.12.34
Keywords: Cc:

Description

while (dir_scan(file, dir, sizeof(file))) {

should be replaced by:

while (!dir_scan(file, dir, sizeof(file))) {

dir_scan returns zero if successfully reads next dir entry.

Change History (3)

comment:1 Changed 12 years ago by smoe

I can confirm the issue to be still present with 7.0.34 and to effect also lib/crypt.cpp. The patch below (compiles here) presumably fixes both. When e.g. applied on empty directories, like while changing projects, possibly, I could imagine the code to have side effects that might contribute to https://boinc.berkeley.edu/trac/ticket/1203 . Just a hunch.

Index: boinc/client/cs_files.cpp

===================================================================
--- boinc.orig/client/cs_files.cpp      2012-08-30 21:03:35.490956131 +0200
+++ boinc/client/cs_files.cpp   2012-09-02 10:40:36.478032727 +0200
@@ -90,12 +90,12 @@
  Is app signed by one of the Application Certifiers?
 

 bool FILE_INFO::verify_file_certs() {
-    char file[256];
+    char file[MAXPATHLEN];
     bool retval = false;
 
     if (!is_dir(CERTIFICATE_DIRECTORY)) return false;
     DIRREF dir = dir_open(CERTIFICATE_DIRECTORY);
-    while (dir_scan(file, dir, sizeof(file))) {
+    while (!dir_scan(file, dir, sizeof(file))) {
         if (cert_verify_file(cert_sigs, file, CERTIFICATE_DIRECTORY)) {
             msg_printf(project, MSG_INFO,
                 "Signature verified using certificate %s", file
@@ -156,7 +156,7 @@
 int FILE_INFO::verify_file(
     bool verify_contents, bool show_errors, bool allow_async
 ) {
-    char cksum[64], pathname[256];
+    char cksum[64], pathname[MAXPATHLEN];
     bool verified;
     int retval;
     double size, local_nbytes;
@@ -177,7 +177,9 @@
    
     if (download_gzipped && !boinc_file_exists(pathname)) {
         char gzpath[MAXPATHLEN];
-        sprintf(gzpath, "%s.gz", pathname);
+        snprintf(gzpath, sizeof(gzpath), "%s.gz", pathname);
+      
FIXME: a distinction is missing for the case that the .gz suffix goes beyond the MAXPATHLEN and is hence not found because
+              one should then not reperform the download as intended below
         if (boinc_file_exists(gzpath) ) {
                        if (allow_async && nbytes > ASYNC_FILE_THRESHOLD) {
                                ASYNC_VERIFY* avp = new ASYNC_VERIFY;
Index: boinc/lib/crypt.cpp
===================================================================
--- boinc.orig/lib/crypt.cpp    2012-09-02 10:27:39.420339596 +0200
+++ boinc/lib/crypt.cpp 2012-09-02 10:42:50.568254414 +0200
@@ -604,7 +604,7 @@
     DIRREF dir = dir_open(certPath);
 
     char file[MAXPATHLEN];
-    while (dir_scan(file, dir, sizeof(file))) {
+    while (!dir_scan(file, dir, sizeof(file))) {
         char fpath[MAXPATHLEN];
            snprintf(fpath, sizeof(fpath), "%s/%s", certPath, file);
        
TODO : replace '128'

comment:2 Changed 12 years ago by davea

Resolution: fixed
Status: newclosed

(In [26071]) - client: fix bug that broke file signing with X.509 certificates.

From matszpk. Fixes #1168.

comment:3 Changed 12 years ago by romw

(In [26094]) - client: fix bug that broke file signing with X.509 certificates.

From matszpk. Fixes #1168.

Note: See TracTickets for help on using tickets.