Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Serious memory bugs parsing provider file #1

Closed
richfelker opened this issue Apr 1, 2020 · 2 comments · Fixed by #2
Closed

Serious memory bugs parsing provider file #1

richfelker opened this issue Apr 1, 2020 · 2 comments · Fixed by #2
Assignees
@fmount
Labels
bug

Comments

@richfelker
Copy link

In lib/parser.c split_str, strings are not null-terminated and insufficient space is allocated to terminate them. This prevents the functionality from working at all unless you "get lucky".
@richfelker
Copy link
Author

In addition, strlen of these unterminated strings is used as a length argument to memcpy, resulting in buffer overflows.

@fmount
Copy link
Owner

fmount commented Apr 4, 2020

Hi,
first, thanks for opening this issue and you're right, that's a te|ho(rrible) bug.
I already started working on it and fixing the evident memory leak(s) you
mentioned.
I have to admit I found several memory leaks and I tried to fix everything
with the next change, trying to avoid abusing about heap and variables
allocation and simplifying the code base as well.

In the next patch I'm going to submit you will find several fixes and some
enhancement like:

  1. file descriptor not closed;
  2. memory not properly freed, rework of split_str function;
  3. use of signal handling to properly free the in-used memory when -s opt
    is passed and SIGINT is received
  4. printing function enhancement to support the very basic json like output
    (and properly document it)

The issue of split_str was already fixed in the gpg branch, but tbh I had no
time to finish it, test and think about merging that feature in master; then,
master wasn't stable enough and the issue you've found is a great example
of that.

Something about testing

Here [1] a simple output related to some testing performed after producing
the fixes described above (run with the following command):

>> valgrind  -s --tool=memcheck --leak-check=full --leak-resolution=high \
    --show-leak-kinds=all --show-reachable=yes ./c_otp -f providerrc.sample -m 1 -s

Feel free to take a look, comment or chime in (and drive me) if you have ideas to optimize the code base of produce fixes for both style or features.

And again, thanks a lot for pointing me out to this bug.

[1] https://pastebin.com/DpKQiVEF

fmount added a commit that referenced this issue Apr 4, 2020
@fmount
Serious memory bugs parsing provider file
dcff6b5 
This patch represents a rework of the main parsing
providerrc function which is afftected by serious
memory leaks. The purpose of the fix is to revisit
the code base providing a few improvements and
optimizations in the way the memory structures are
allocated, used and freed.
Finally, the print function is improved and now it
allows to get some basic json like output.

Closes: #1
Signed-off-by: fmount <fmount@inventati.org>
fmount added a commit that referenced this issue Apr 4, 2020
@fmount
Serious memory bugs parsing provider file
d17f345 
This patch represents a rework of the main parsing
providerrc function which is afftected by serious
memory leaks. The purpose of the fix is to revisit
the code base providing a few improvements and
optimizations in the way the memory structures are
allocated, used and freed.
Finally, the print function is improved and now it
allows to get some basic json like output.

Closes: #1
Signed-off-by: fmount <fmount@inventati.org>
@fmount fmount mentioned this issue Apr 4, 2020
Merged
@fmount fmount added the bug label Apr 4, 2020
@fmount fmount self-assigned this Apr 4, 2020
fmount added a commit that referenced this issue Apr 6, 2020
@fmount
Serious memory bugs parsing provider file
0cd59ec 
This patch represents a rework of the main parsing
providerrc function which is afftected by serious
memory leaks. The purpose of the fix is to revisit
the code base providing a few improvements and
optimizations in the way the memory structures are
allocated, used and freed.
Finally, the print function is improved and now it
allows to get some basic json like output.

Closes: #1
Signed-off-by: fmount <fmount@inventati.org>
fmount added a commit that referenced this issue Apr 6, 2020
@fmount
Serious memory bugs parsing provider file
397d4cd 
This patch represents a rework of the main parsing
providerrc function which is afftected by serious
memory leaks. The purpose of the fix is to revisit
the code base providing a few improvements and
optimizations in the way the memory structures are
allocated, used and freed.
Finally, the print function is improved and now it
allows to get some basic json like output.
The main function is reworked as well, the switch
case statement is turned into a section which can
be used only for getting input and validating it.
A new flow is then added to continue the execution
according to the cli parameters:
if -b<key> is received it has the highest priority
over the other cli options (single shot mode), or
the otp computing is run in loop mode (if '-s' is
specified).

Closes: #1
Signed-off-by: fmount <fmount@inventati.org>

The switch-case statement in the main function should
be used for processing cli parameters only.

Signed-off-by: fmount <fmount@inventati.org>
fmount added a commit that referenced this issue Apr 6, 2020
@fmount
Serious memory bugs parsing provider file
e291708 
This patch represents a rework of the main parsing
providerrc function which is afftected by serious
memory leaks. The purpose of the fix is to revisit
the code base providing a few improvements and
optimizations in the way the memory structures are
allocated, used and freed.
Finally, the print function is improved and now it
allows to get some basic json like output.
The main function is reworked as well, the switch
case statement is turned into a section which can
be used only for getting input and validating it.
A new flow is then added to continue the execution
according to the cli parameters:
if -b<key> is received it has the highest priority
over the other cli options (single shot mode), or
the otp computing is run in loop mode (if '-s' is
specified).

Closes: #1
Signed-off-by: fmount <fmount@inventati.org>
@fmount fmount closed this as completed in #2 Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fmount fmount

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Serious memory bugs parsing provider file fmount/c_otp
2 participants
@fmount @richfelker