Use Github Actions CodeQL tests

IB-7528 Signed-off-by: Raul Metsma <raul@metsma.ee>
open-eid · Dec 20, 2022 · e584a78 · e584a78
1 parent e513b3b
commit e584a78
Show file tree

Hide file tree

Showing 11 changed files with 118 additions and 71 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -255,3 +255,48 @@ jobs:
           --form version=master \
           --form description="Github Actions CI build" \
           https://scan.coverity.com/builds?project=$PROJECTNAME
+  codeql:
+    name: Run CodeQL tests
+    if: github.repository == 'open-eid/libdigidocpp'
+    runs-on: ubuntu-20.04
+    permissions:
+      security-events: write
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v3
+      with:
+        submodules: recursive
+    - name: Install dependencies
+      run: sudo apt update -qq && sudo apt install --no-install-recommends -y cmake vim-common xsdcxx libxml-security-c-dev zlib1g-dev curl ca-certificates
+    - name: Initialize CodeQL
+      uses: github/codeql-action/init@v2
+      with:
+        languages: cpp
+        queries: +security-and-quality
+    - name: Build
+      run: |
+        cmake -B build -S . \
+          -DSWIG_EXECUTABLE=NOTFOUND \
+          -DBoost_INCLUDE_DIR=NOTFOUND \
+          -DDOXYGEN_EXECUTABLE=NOTFOUND \
+          -DBUILD_TOOLS=NO
+        cmake --build build
+    - name: Perform CodeQL Analysis
+      uses: github/codeql-action/analyze@v2
+      with:
+        upload: False
+        output: sarif-results
+    - name: Filter results
+      uses: advanced-security/filter-sarif@develop
+      with:
+        patterns: |
+          -src/json.hpp
+          -src/minizip/*
+          -build/src/xml/*
+          -**:cpp/poorly-documented-function
+        input: sarif-results/cpp.sarif
+        output: sarif-results/cpp.sarif
+    - name: Upload results
+      uses: github/codeql-action/upload-sarif@v2
+      with:
+        sarif_file: sarif-results/cpp.sarif
diff --git a/README.md b/README.md
@@ -9,7 +9,6 @@
 ## Building
 [![Build Status](https://github.com/open-eid/libdigidocpp/workflows/CI/badge.svg?branch=master)](https://github.com/open-eid/libdigidocpp/actions)
 [![Coverity Scan Build Status](https://scan.coverity.com/projects/727/badge.svg)](https://scan.coverity.com/projects/727)
-[![LGTM alerts](https://img.shields.io/lgtm/alerts/g/open-eid/libdigidocpp.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/open-eid/libdigidocpp/alerts/)
 
 ### Ubuntu, Fedora
 

diff --git a/src/ASiContainer.cpp b/src/ASiContainer.cpp
@@ -37,7 +37,7 @@ using namespace digidoc;
 using namespace digidoc::util;
 using namespace std;
 
-constexpr unsigned long MAX_MEM_FILE = 500*1024*1024;
+constexpr unsigned long MAX_MEM_FILE = 500UL*1024UL*1024UL;
 
 class ASiContainer::Private
 {
@@ -91,8 +91,8 @@ unique_ptr<ZipSerialize> ASiContainer::load(const string &path, bool mimetypeReq
     if(mimetypeRequired && list[0] != "mimetype")
         THROW("required mimetype not found");
 
-    // ETSI TS 102 918: mimetype has to be the first in the archive;
-    if(list[0] == "mimetype")
+    // ETSI TS 102 918: mimetype has to be the first in the archive
+    if(list.front() == "mimetype")
     {
         stringstream data;
         z->extract(list.front(), data);
@@ -178,20 +178,19 @@ void ASiContainer::addDataFile(const string &path, const string &mediaType)
     if(!File::fileExists(path))
         THROW("Document file '%s' does not exist.", path.c_str());
 
-    ZipSerialize::Properties prop = { appInfo(), File::modifiedTime(path), File::fileSize(path) };
-    zproperty(File::fileName(path), prop);
+    ZipSerialize::Properties prop { appInfo(), File::modifiedTime(path), File::fileSize(path) };
+    bool useTempFile = prop.size > MAX_MEM_FILE;
+    zproperty(File::fileName(path), move(prop));
     unique_ptr<istream> is;
-    if(prop.size > MAX_MEM_FILE)
+    if(useTempFile)
     {
         is = make_unique<ifstream>(File::encodeName(path).c_str(), ifstream::binary);
     }
     else
     {
-        ifstream file(File::encodeName(path).c_str(), ifstream::binary);
         stringstream *data = new stringstream;
-        if(file)
+        if(ifstream file(File::encodeName(path).c_str(), ifstream::binary); file)
             *data << file.rdbuf();
-        file.close();
         is.reset(data);
     }
     addDataFilePrivate(move(is), fileName, mediaType);
@@ -235,7 +234,7 @@ void ASiContainer::removeDataFile(unsigned int id)
     if(!d->signatures.empty())
         THROW("Can not remove document from container which has signatures, remove all signatures before removing document.");
     if(id >= d->documents.size())
-        THROW("Incorrect document id %u, there are only %lu documents in container.", id, (unsigned long)dataFiles().size());
+        THROW("Incorrect document id %u, there are only %zu documents in container.", id, dataFiles().size());
     vector<DataFile*>::const_iterator it = (d->documents.cbegin() + id);
     delete *it;
     d->documents.erase(it);
@@ -256,16 +255,15 @@ Signature* ASiContainer::addSignature(unique_ptr<Signature> &&signature)
 void ASiContainer::removeSignature(unsigned int id)
 {
     if(id >= d->signatures.size())
-        THROW("Incorrect signature id %u, there are only %lu signatures in container.", id, (unsigned long)d->signatures.size());
+        THROW("Incorrect signature id %u, there are only %zu signatures in container.", id, d->signatures.size());
     vector<Signature*>::const_iterator it = (d->signatures.cbegin() + id);
     delete *it;
     d->signatures.erase(it);
 }
 
 void ASiContainer::deleteSignature(Signature* s)
 {
-    vector<Signature*>::const_iterator i = find(d->signatures.cbegin(), d->signatures.cend(), s);
-    if(i != d->signatures.cend())
+    if(auto i = find(d->signatures.cbegin(), d->signatures.cend(), s); i != d->signatures.cend())
         d->signatures.erase(i);
     delete s;
 }
@@ -288,9 +286,9 @@ ZipSerialize::Properties ASiContainer::zproperty(const string &file) const
     return d->properties[file] = { appInfo(), time(nullptr), 0 };
 }
 
-void ASiContainer::zproperty(const string &file, const ZipSerialize::Properties &prop)
+void ASiContainer::zproperty(const string &file, ZipSerialize::Properties &&prop)
 {
-    d->properties[file] = prop;
+    d->properties[file] = move(prop);
 }
 
 /**
@@ -310,7 +308,7 @@ string ASiContainer::readMimetype(istream &is)
        (bom[0] == 0xEF && bom[1] == 0xFF))
         THROW("Mimetype file must be UTF-8 format.");
     // does not contain UTF-8 BOM reset pos
-    if(!(bom[0] == 0xEF && bom[1] == 0xBB && bom[2] == 0xBF))
+    if(bom[0] != 0xEF || bom[1] != 0xBB || bom[2] != 0xBF)
         is.seekg(0, ios::beg);
 
     string text;

diff --git a/src/ASiContainer.h b/src/ASiContainer.h
@@ -70,7 +70,7 @@ namespace digidoc
           void zpath(const std::string &file);
           std::string zpath() const;
           ZipSerialize::Properties zproperty(const std::string &file) const;
-          void zproperty(const std::string &file, const ZipSerialize::Properties &prop);
+          void zproperty(const std::string &file, ZipSerialize::Properties &&prop);
 
           static std::string readMimetype(std::istream &path);
 

diff --git a/src/Conf.cpp b/src/Conf.cpp
@@ -174,11 +174,7 @@ bool Conf::TSLAutoUpdate() const { return true; }
  */
 string Conf::TSLCache() const
 {
-#ifdef _WIN32
-    return File::env("APPDATA") + "\\digidocpp\\tsl\\";
-#else
-    return File::env("HOME") + "/.digidocpp/tsl/";
-#endif
+    return File::digidocppPath() + "tsl";
 }
 
 /**

diff --git a/src/SiVaContainer.cpp b/src/SiVaContainer.cpp
@@ -202,7 +202,7 @@ SiVaContainer::SiVaContainer(const string &path, const string &ext, bool useHash
     {
         Exception e(EXCEPTION_PARAMS("Signature validation"));
         for(const json &error: result["requestErrors"])
-            EXCEPTION_ADD(e, error.value<string>("message", {}).c_str());
+            EXCEPTION_ADD(e, "%s", error.value<string>("message", {}).c_str());
         throw e;
     }
 
@@ -260,13 +260,13 @@ SiVaContainer::SiVaContainer(const string &path, const string &ext, bool useHash
         {
             string message = error["content"];
             if(message.find("Bad digest for DataFile") == 0 && useHashCode)
-                THROW(message.c_str());
-            s->_exceptions.emplace_back(EXCEPTION_PARAMS(message.c_str()));
+                THROW("%s", message.c_str());
+            s->_exceptions.emplace_back(EXCEPTION_PARAMS("%s", message.c_str()));
         }
         for(const json &warning: signature.value<json>("warnings", {}))
         {
             string message = warning["content"];
-            Exception ex(EXCEPTION_PARAMS(message.c_str()));
+            Exception ex(EXCEPTION_PARAMS("%s", message.c_str()));
             if(message == "X509IssuerName has none or invalid namespace: null" ||
                 message == "X509SerialNumber has none or invalid namespace: null")
                 ex.setCode(Exception::IssuerNameSpaceWarning);

diff --git a/src/XmlConf.cpp b/src/XmlConf.cpp
@@ -127,15 +127,9 @@ XmlConf::Private::Private(const string &path, string schema)
         }
     }
 
-#ifdef _WIN32
-    USER_CONF_LOC = File::env("APPDATA");
+    USER_CONF_LOC = File::digidocppPath();
     if (!USER_CONF_LOC.empty())
-        USER_CONF_LOC += "\\digidocpp\\digidocpp.conf";
-#else
-    USER_CONF_LOC = File::env("HOME");
-    if (!USER_CONF_LOC.empty())
-        USER_CONF_LOC += "/.digidocpp/digidocpp.conf";
-#endif
+        USER_CONF_LOC += "digidocpp.conf";
 
     if(path.empty())
     {

diff --git a/src/crypto/TSL.cpp b/src/crypto/TSL.cpp
@@ -193,7 +193,7 @@ vector<TSL::Service> TSL::services() const
 
 void TSL::debugException(const digidoc::Exception &e)
 {
-    Log::out(Log::DebugType, e.file().c_str(), e.line(), e.msg().c_str());
+    Log::out(Log::DebugType, e.file().c_str(), e.line(), "%s", e.msg().c_str());
     for(const Exception &ex: e.causes())
         debugException(ex);
 }

diff --git a/src/crypto/X509CertStore.cpp b/src/crypto/X509CertStore.cpp
@@ -203,13 +203,16 @@ int X509CertStore::validate(int ok, X509_STORE_CTX *ctx, const Type &type)
     case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY:
     case X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE:
     case X509_V_ERR_CERT_UNTRUSTED:
+        break;
+    default: return ok;
+    }
+
+    X509 *x509 = X509_STORE_CTX_get0_cert(ctx);
+    for(const TSL::Service &s: *instance()->d)
     {
-        X509 *x509 = X509_STORE_CTX_get0_cert(ctx);
-        for(const TSL::Service &s: *instance()->d)
-        {
-            if(type.find(s.type) == type.cend())
-                continue;
-            if(none_of(s.certs.cbegin(), s.certs.cend(), [&](const X509Cert &issuer){
+        if(type.find(s.type) == type.cend())
+            continue;
+        if(none_of(s.certs.cbegin(), s.certs.cend(), [&](const X509Cert &issuer){
                 if(issuer == x509)
                     return true;
                 if(X509_check_issued(issuer.handle(), x509) != X509_V_OK)
@@ -220,25 +223,22 @@ int X509CertStore::validate(int ok, X509_STORE_CTX *ctx, const Type &type)
                 OpenSSLException(EXCEPTION_PARAMS("ignore")); //Clear errors
                 return false;
             }))
-                continue;
-            X509_STORE_CTX_set_ex_data(ctx, 0, const_cast<TSL::Validity*>(&s.validity[0]));
-            X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx);
-            if(!(X509_VERIFY_PARAM_get_flags(param) & X509_V_FLAG_USE_CHECK_TIME) || s.validity.empty())
-                return 1;
-            for(const TSL::Validity &v: s.validity)
+            continue;
+        X509_STORE_CTX_set_ex_data(ctx, 0, const_cast<TSL::Validity*>(&s.validity[0]));
+        X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx);
+        if(!(X509_VERIFY_PARAM_get_flags(param) & X509_V_FLAG_USE_CHECK_TIME) || s.validity.empty())
+            return 1;
+        for(const TSL::Validity &v: s.validity)
+        {
+            if(X509_VERIFY_PARAM_get_time(param) >= v.start &&
+                (v.end == 0 || X509_VERIFY_PARAM_get_time(param) <= v.end))
             {
-                if(X509_VERIFY_PARAM_get_time(param) >= v.start &&
-                    (v.end == 0 || X509_VERIFY_PARAM_get_time(param) <= v.end))
-                {
-                    X509_STORE_CTX_set_ex_data(ctx, 0, const_cast<TSL::Validity*>(&v));
-                    return 1;
-                }
+                X509_STORE_CTX_set_ex_data(ctx, 0, const_cast<TSL::Validity*>(&v));
+                return 1;
             }
         }
-        return ok;
-    }
-    default: return ok;
     }
+    return ok;
 }
 
 /**
@@ -258,13 +258,9 @@ bool X509CertStore::verify(const X509Cert &cert, bool noqscd) const
     {
         int err = X509_STORE_CTX_get_error(csc.get());
         OpenSSLException e(EXCEPTION_PARAMS("%s", X509_verify_cert_error_string(err)));
-        switch(err)
-        {
-        case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY:
+        if(err == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY)
             e.setCode(Exception::CertificateIssuerMissing);
-            throw e;
-        default: throw e;
-        }
+        throw e;
     }
 
     if(noqscd)

diff --git a/src/util/File.cpp b/src/util/File.cpp
@@ -29,12 +29,15 @@
 #include <sys/types.h>
 
 #ifdef _WIN32
+#include <filesystem>
 #include <Windows.h>
+#include <ShlObj_core.h>
 #include <direct.h>
 #include <sys/utime.h>
 #else
 #include <dirent.h>
 #include <sys/param.h>
+#include <pwd.h>
 #include <unistd.h>
 #include <utime.h>
 #endif
@@ -168,9 +171,7 @@ File::f_string File::encodeName(string_view fileName)
     if(fileName.empty())
         return {};
 #if defined(_WIN32)
-    int len = MultiByteToWideChar(CP_UTF8, 0, fileName.data(), int(fileName.size()), nullptr, 0);
-    f_string out(size_t(len), 0);
-    len = MultiByteToWideChar(CP_UTF8, 0, fileName.data(), int(fileName.size()), out.data(), len);
+    f_string out = std::filesystem::u8path(fileName);
 #elif defined(__APPLE__)
     CFMutableStringRef ref = CFStringCreateMutable(nullptr, 0);
     CFStringAppendCString(ref, fileName.data(), kCFStringEncodingUTF8);
@@ -198,9 +199,7 @@ string File::decodeName(const f_string_view &localFileName)
     if(localFileName.empty())
         return {};
 #if defined(_WIN32)
-    int len = WideCharToMultiByte(CP_UTF8, 0, localFileName.data(), int(localFileName.size()), nullptr, 0, nullptr, nullptr);
-    string out(size_t(len), 0);
-    WideCharToMultiByte(CP_UTF8, 0, localFileName.data(), int(localFileName.size()), out.data(), len, nullptr, nullptr);
+    string out = std::filesystem::path(localFileName).u8string();
 #elif defined(__APPLE__)
     CFMutableStringRef ref = CFStringCreateMutable(nullptr, 0);
     CFStringAppendCString(ref, localFileName.data(), kCFStringEncodingUTF8);
@@ -342,7 +341,7 @@ string File::directory(const string& path)
 string File::path(const string& directory, const string& relativePath)
 {
     string dir(directory);
-    if(!dir.empty() && (dir[dir.size() - 1] == '/' || dir[dir.size() - 1] == '\\'))
+    if(!dir.empty() && (dir.back() == '/' || dir.back() == '\\'))
         dir.pop_back();
 
     string path = dir + "/" + relativePath;
@@ -391,7 +390,7 @@ void File::createDirectory(const string& path)
         THROW("Can not create directory with no name.");
 
     string dirPath = path;
-    if(dirPath[dirPath.size() - 1] == '/' || dirPath[dirPath.size() - 1] == '\\')
+    if(dirPath.back() == '/' || dirPath.back() == '\\')
         dirPath.pop_back();
 
     f_string _path = encodeName(dirPath);
@@ -414,6 +413,25 @@ void File::createDirectory(const string& path)
         THROW("Failed to create directory '%s'", path.c_str());
 }
 
+std::string File::digidocppPath()
+{
+#ifdef _WIN32
+    PWSTR knownFolder = 0;
+    if(SHGetKnownFolderPath(FOLDERID_RoamingAppData, KF_FLAG_CREATE, nullptr, &knownFolder) != S_OK)
+        THROW("Failed to get home directory");
+    std::wstring appData = knownFolder;
+    CoTaskMemFree(knownFolder);
+    return path(decodeName(appData), "digidocpp");
+#else
+    string buf(sysconf(_SC_GETPW_R_SIZE_MAX), 0);
+    struct passwd pwbuf {};
+    struct passwd *pw = nullptr;
+    if(getpwuid_r(getuid(), &pwbuf, buf.data(), buf.size(), &pw) != 0 || !pw)
+        THROW("Failed to get home directory");
+    return path(pw->pw_dir, "/.digidocpp");
+#endif
+}
+
 /**
  * Returns true if the path is relative
  *

diff --git a/src/util/File.h b/src/util/File.h
@@ -46,6 +46,7 @@ namespace digidoc
               static bool fileExists(const std::string& path);
               static f_string encodeName(std::string_view fileName);
               static std::string decodeName(const f_string_view &localFileName);
+              static std::string digidocppPath();
               static bool isRelative(const std::string &path);
               static time_t modifiedTime(const std::string &path);
               static void updateModifiedTime(const std::string &path, time_t time);