From bc9d9820d2fca7a5fbca77669cea178b55c9700f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarom=C3=ADr=20Wysoglad?= Date: Wed, 2 Aug 2023 17:59:30 +0200 Subject: [PATCH 1/3] Fix buffer overflow (#27) * Fix buffer overflow. Before this, the amqp messages would get copied to ring buffer without checking, if there is enough space in the buffer. That would lead to buffer overflows for long messages. Now the sizes are checked and if the message is longer than the buffer, it's first read from the bus and then it's discarded. * Committing clang-format changes --------- Co-authored-by: InfraWatch CI --- amqp_rcv_th.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/amqp_rcv_th.c b/amqp_rcv_th.c index d6d3357..514ac9d 100644 --- a/amqp_rcv_th.c +++ b/amqp_rcv_th.c @@ -55,6 +55,7 @@ static void handle_receive(app_data_t *app, pn_event_t *event, if (pn_delivery_readable(d)) { pn_link_t *l = pn_delivery_link(d); size_t size = pn_delivery_pending(d); + bool too_long = false; pn_rwbytes_t *m = rb_get_head(app->rbin); /* Append data to incoming message buffer */ @@ -63,7 +64,19 @@ static void handle_receive(app_data_t *app, pn_event_t *event, // First time through m->size = 0 for a partial message... size_t oldsize = m->size; m->size += size; - recv = pn_link_recv(l, m->start + oldsize, size); + if (m->size >= app->ring_buffer_size) { + fprintf(stderr, + "Message too long: %ldB >= %dB.\n" + "You may want to increase the ring buffer size.\n", + m->size, app->ring_buffer_size); + // I can't figure out how to stop processing the delivery + // without reading everything, so just read from the start + // of the buffer and discard it later. + recv = pn_link_recv(l, m->start, size); + too_long = true; + } else { + recv = pn_link_recv(l, m->start + oldsize, size); + } if (recv == PN_ABORTED) { printf("Message aborted\n"); fflush(stdout); @@ -77,9 +90,13 @@ static void handle_receive(app_data_t *app, pn_event_t *event, pn_link_close(l); /* Unexpected error, close the link */ } else if (!pn_delivery_partial(d)) { /* Message is complete */ // Place in the ring buffer HERE - rb_put(app->rbin); + if (too_long) { + m->size = 0; /* Forget the data we accumulated */ + } else { + rb_put(app->rbin); + app->amqp_received++; + } - app->amqp_received++; pn_delivery_update(d, PN_ACCEPTED); pn_delivery_settle(d); /* settle and free d */ From ede398f1c3ce7977e3d22a71edd4d7672cae5806 Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Thu, 3 Aug 2023 15:26:02 -0400 Subject: [PATCH 2/3] [testing] Update linter to more recent component versions (#29) * Update base image for GHA test and linter versions * annocheck should run in verbose mode * Try using -flto flag to build * Try adding -fplugin=annobin to better cover code * Add gcc-plugin-annobin package to deployment * Generate missing build notes so annocheck passes * Final set of changes based on feedback After further investigation it was found that the Makefile was missing calls to CFLAGS which was causing issues with notes generation. That has now been fixed. Also the CFLAGS and LDFLAGS have been updated to match the latest best values provided to us by release-delivery. Additionally the CFLAGS and LDFLAGS configuration have been moved out of the external build_checks.sh script into the Makefile directly so that the build_checks script can just run make directly, as well as setting proper defaults for how we expect sg-bridge to be built. Signed-off-by: Leif Madsen --------- Signed-off-by: Leif Madsen --- .github/workflows/linter.yml | 8 ++++---- Makefile | 8 ++++---- build/build_checks.sh | 9 +++------ 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index 0c7235f..59b6354 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -5,11 +5,11 @@ on: push jobs: clang-lint: name: Lint code base - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - name: Checkout code - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: Run clang-format-lint uses: DoozyX/clang-format-lint-action@v0.11 with: @@ -18,9 +18,9 @@ jobs: clangFormatVersion: 10 inplace: True - name: Check build - run: docker run -i --volume $GITHUB_WORKSPACE:/src/sg-bridge:z --workdir /src/sg-bridge fedora:32 /bin/sh -c 'sh ./build/build_checks.sh' + run: docker run -i --volume $GITHUB_WORKSPACE:/src/sg-bridge:z --workdir /src/sg-bridge fedora:latest /bin/sh -c 'sh ./build/build_checks.sh' - name: Commit in-place changes based on linting recommendations - uses: EndBug/add-and-commit@v4 + uses: EndBug/add-and-commit@v9 with: author_name: InfraWatch CI author_email: robot@infra.watch diff --git a/Makefile b/Makefile index f96a15b..197b4fc 100644 --- a/Makefile +++ b/Makefile @@ -22,9 +22,9 @@ $(shell mkdir -p $(dir $(OBJS)) >/dev/null) $(shell mkdir -p $(dir $(DEPS)) >/dev/null) CC=gcc -CFLAGS+=-Wall -O3 -std=gnu99 +CFLAGS+=-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64-v2 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection LDLIBS=-lqpid-proton -lpthread -LDFLAGS+= +LDFLAGS+=-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 DEPFLAGS = -MT $@ -MD -MP -MF $(DEPDIR)/$*.Td @@ -32,7 +32,7 @@ DEPFLAGS = -MT $@ -MD -MP -MF $(DEPDIR)/$*.Td COMPILE.c = $(CC) $(DEPFLAGS) $(CFLAGS) $(CPPFLAGS) -c -o $@ # link object files to binary -LINK.o = $(LD) $(LDFLAGS) $(LDLIBS) -o $@ +LINK.o = $(LD) $(LDFLAGS) $(CFLAGS) $(LDLIBS) -o $@ # precompile step PRECOMPILE = @@ -62,7 +62,7 @@ image: version-check @echo 'Done.' $(BIN): $(OBJS) - $(CC) -o $@ $^ $(LDFLAGS) $(LDLIBS) + $(CC) -o $@ $^ $(LDFLAGS) $(CFLAGS) $(LDLIBS) $(OBJDIR)/%.o: %.c $(OBJDIR)/%.o: %.c $(DEPDIR)/%.d diff --git a/build/build_checks.sh b/build/build_checks.sh index 7f688c4..3e50652 100755 --- a/build/build_checks.sh +++ b/build/build_checks.sh @@ -1,9 +1,6 @@ #!/usr/bin/env bash - set -ex -dnf install make gcc qpid-proton-c-devel annobin-annocheck rpm-build -y - -make 'CFLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' 'LDFLAGS=-Wl,-z,relro -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld' - -annocheck bridge +dnf install make gcc qpid-proton-c-devel annobin-annocheck gcc-plugin-annobin rpm-build -y +make +annocheck --verbose --verbose bridge From be893b72405a893829811592510352b8ae01f2bc Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Wed, 16 Aug 2023 10:14:05 -0400 Subject: [PATCH 3/3] Fix and test Dockerfile builds (#31) When making fixes in #29 related to lint checks, a local build wasn't done against the Dockerfile, which has a different base image from that used by build_checks for lint verification. The result of that was that building sg-bridge with the Dockerfile failed on gcc compiling due to hardening instructions not being available. This commit does the following: * adds the RPM dependency to the build layer of Dockerfile to install hardening instructions for gcc * changes the default -march CFLAG to be x86-64 instead of x86-64-v2 which fails in ubi8 * updates the GHA to add a build check against the Dockerfile and not just run build_checks.sh for future validation Found by csibbitt and noted in chat. Resolves #30 Signed-off-by: Leif Madsen --- .github/workflows/linter.yml | 9 +++++++++ Makefile | 4 ++-- build/Dockerfile | 5 ++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index 59b6354..74833f5 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -28,3 +28,12 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + docker-build-check: + name: Check Dockerfile results in successful build + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v3 + - name: Check build of Dockerfile is successful + run: docker build -t sg-bridge:check-build -f build/Dockerfile . diff --git a/Makefile b/Makefile index 197b4fc..9ea424f 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ MAJOR?=0 MINOR?=1 - + VERSION=$(MAJOR).$(MINOR) BIN := bridge @@ -22,7 +22,7 @@ $(shell mkdir -p $(dir $(OBJS)) >/dev/null) $(shell mkdir -p $(dir $(DEPS)) >/dev/null) CC=gcc -CFLAGS+=-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64-v2 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection +CFLAGS+=-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection LDLIBS=-lqpid-proton -lpthread LDFLAGS+=-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 diff --git a/build/Dockerfile b/build/Dockerfile index 16455b4..0cab30b 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -4,8 +4,11 @@ FROM registry.access.redhat.com/ubi8 AS builder # dependencies for qpid-proton-c COPY build/repos/opstools.repo /etc/yum.repos.d/opstools.repo +# redhat-rpm-config is required to provide hardening compiling instructions +# (such as /usr/lib/rpm/redhat/redhat-hardened-cc1) even though we're not +# building RPMs here RUN dnf install qpid-proton-c-devel --setopt=tsflags=nodocs -y && \ - dnf install gcc make -y && \ + dnf install gcc make redhat-rpm-config -y && \ dnf clean all ENV D=/home/bridge