OpenBSD Journal

informIT: Writing Insecure C

Contributed by ray on from the don't-blame-the-tools dept.

informIT published three articles on pitfalls of writing C code and how to avoid them.

Use of the C programming language is often blamed for insecure code. This is not entirely a valid accusation; projects like OpenBSD show that it is possible to write secure code in C. The problem with C, in this respect, is the same as the problem with assembly-language programming: The language exposes all of the features of the architecture to you, but little else. It provides all the features you need to write tools for secure coding, but doesn't provide these tools itself.

This series will look at some of the common causes of errors in C code and how to avoid them.

The articles are: Part 1: Error Checking, Part 2: Integer Issues, and finally Part 3: Buffers and Strings.

(Comments are closed)


Comments
  1. By kamper (66.207.218.21) on

    "Exceptions have one significant advantage, however: You can't passively ignore them."

    That's not really true. Or at least it's only true of checked exceptions. In java, for instance, most exceptions are checked but you can easily catch a checked exception and wrap it in an unchecked exception and people that call your code can easily ignore it (until it gets thrown).

    Iirc, in .net, all exceptions are unchecked and I believe that is the trend with most other languages.

    That said, I have little faith that the average human coder can/will cover all error scenarios effectively and having the system throw, say, a NullPointerException or DivideByZeroException or ArrayOutOfBoundsException rather than segfaulting allows for the possibility of a slightly more elegant recovery. Of course, most programmers just toss the exceptions into a log that nobody will ever read and never fix the problem... I guess we haven't come up with a language that fixes slacking yet.

    Comments
    1. By dbt (66.201.44.122) dbt_at_meat.net on http://meat.net/

      "That's not really true. Or at least it's only true of checked exceptions. In java, for instance, most exceptions are checked but you can easily catch a checked exception and wrap it in an unchecked exception and people that call your code can easily ignore it (until it gets thrown)."

      You can ignore unchecked exceptions at compile time, but they still must be dealt with at runtime. In C++ it will eventually call terminate(), in C# or Java it'll kill the thread (and exit if it's the main thread).

      Comments
      1. By kamper (66.207.218.21) on

        > "That's not really true. Or at least it's only true of checked exceptions. In java, for instance, most exceptions are checked but you can easily catch a checked exception and wrap it in an unchecked exception and people that call your code can easily ignore it (until it gets thrown)."
        >
        > You can ignore unchecked exceptions at compile time, but they still must be dealt with at runtime. In C++ it will eventually call terminate(), in C# or Java it'll kill the thread (and exit if it's the main thread).

        Sure, and you can ignore a NULL return from malloc() at compile time. The point is, unless the language has no unchecked exceptions (which I don't think is even logically feasible) you can very easily "passively ignore them."

  2. By Anonymous Coward (195.83.212.221) on

    In Part I, the author recommends using the following macro to check if malloc returns an error :

    #define MALLOC(x,y) do { y malloc(x); if (!y) abort(1); } while(0)

    Isn't something wrong with that code ? when is "y" assigned the result of the malloc ?

    In any case, man 3 malloc has this to say :

    When using malloc() be careful to avoid the following idiom:

    if ((p = malloc(num * size)) == NULL)
    err(1, "malloc");

    The multiplication may lead to an integer overflow. To avoid this,
    calloc() is recommended.

    Thanks for telling us how to write Insecure C !

    Comments
    1. By Rich (195.212.199.56) on

      > In Part I, the author recommends using the following macro to check if malloc returns an error :

      No no no no no!

      Macros are an abomination and should have long ago been consigned to the bin of clever but actually really dangerous and stupid ideas.

      I can't take seriously any article that suggests using macros. What are the authors thinking?

      Comments
      1. By Anonymous Coward (76.90.138.122) on

        > > In Part I, the author recommends using the following macro to check if malloc returns an error :
        >
        > No no no no no!
        >
        > Macros are an abomination and should have long ago been consigned to the bin of clever but actually really dangerous and stupid ideas.
        >
        > I can't take seriously any article that suggests using macros. What are the authors thinking?


        I understand that very large macros can be hard to read and difficult
        to debug, but I don't understand why you think they are an abomination.

        Very short macros are a blessing. Considering the following:

        #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
        #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
        

        These are dangerous and stupid? Why?

        Comments
        1. By Anonymous Coward (195.212.199.56) on

          > These are dangerous and stupid? Why?

          Because (and I know these are very old argument, but they are still true):

          1/ Macros alter the code before the compiler sees it. What you see it NOT what you get. This also causes headaches when developing code

          2/ Debuggers / lint and other tools have problems with them

          3/ They are COMPLETELY unnecessary

        2. By Anonymous Coward (212.77.163.104) on

          > Very short macros are a blessing. Considering the following:
          >
          > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
          > #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
          >
          > These are dangerous and stupid? Why?

          Not stupid but maybe a little bit dangerous, though:
          max = MAX(*array++, max);

          Comments
          1. By Rich (195.212.199.56) on

            > Not stupid but maybe a little bit dangerous, though:
            > max = MAX(*array++, max);

            Quite!

          2. By Anonymous Coward (212.20.215.132) on

            > > Very short macros are a blessing. Considering the following:
            > >
            > > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
            > > #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
            > >
            > > These are dangerous and stupid? Why?
            >
            > Not stupid but maybe a little bit dangerous, though:
            > max = MAX(*array++, max);

            Why would you do that? You know MAX is a macro.

            If you don't know, and you insist on passing expressions with side-effects, make sure you actually call a function; max = (MAX)(*array++, max);

            Comments
            1. By Rich (195.212.199.56) on

              > Why would you do that? You know MAX is a macro.

              A classic statement if ever I heard one :-)

              That's the point isn't it? WHY would anyone know that MAX is a macro? Unless they go and look at its definition? If there was an existing use of it in the code, then one may just replicate the use without checking; it's a trivial "function" after all - there's no real need to check its prototype. And why would use of such a function cause side-effects? It wouldn't, of course. IF it was a function.

              And that's the point. It's NOT obviously a macro. Macros are dangerous and horrible.

              Comments
              1. By Anonymous Coward (212.20.215.132) on

                > > Why would you do that? You know MAX is a macro.
                >
                > That's the point isn't it? WHY would anyone know that MAX is a macro?

                "Convention." It's MAX, not max. You have also read the manual for the
                interface you're using, which says that it's a macro and that arguments
                may be evaluated more than once.

                > [...] it's a trivial "function" after all - there's no real need
                > to check its prototype. And why would use of such a function cause
                > side-effects? It wouldn't, of course. IF it was a function.

                Is putc a function or a macro? It may be both (hence my "convention"
                earlier ;-) ). Does that mean it can evaluate its arguments more than
                once? Yes, it does.

                In fact, any standard C library function may also be defined as a
                macro. However, they cannot evaluate their argument for side-
                effects more than once (getc and putc are the exception), thus having
                function-like behavior.

                And yes, I'm mostly just babbling and not really advocating macros ;-)

                > Macros are dangerous and horrible.

                True, but they're also sometimes useful.

                Comments
                1. By Marc Espie (193.6.223.128) espie@openbsd.org on


                  > True, but they're also sometimes useful.

                  It's very easy to argue that, in the case at hand, they just show
                  shortcomings of the language.

                  There have been inline functions in the language for ages...
                  and languages derived from C where you don't have to use macros for generic programming.

                  In such languages, you just write max(x,y) and let the compiler handle the typechecking... which it does just fine.

                  Macros are a plague, like every other preprocessor, every piece of shit that doesn't understand anything about the target language.

                  An incredible lot of stupid software porting issues has to do with abuse of the preprocessor. The rest usually has to do with autoconf, automake, yet other abuses of other kind of preprocessors.

        3. By Anonymous Coward (4.78.205.35) on

          > > > In Part I, the author recommends using the following macro to check if malloc returns an error :
          > >
          > > No no no no no!
          > >
          > > Macros are an abomination and should have long ago been consigned to the bin of clever but actually really dangerous and stupid ideas.
          > >
          > > I can't take seriously any article that suggests using macros. What are the authors thinking?
          >
          > I understand that very large macros can be hard to read and difficult
          > to debug, but I don't understand why you think they are an abomination.
          >
          > Very short macros are a blessing. Considering the following:
          >
          >
          > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
          > #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
          >
          >
          > These are dangerous and stupid? Why?
          The above could have unexpected side-effects if you do something like this:

          foo = MIN(x++, y--);

          After this runs, x will be incremented twice and y will be decremented twice, instead of once if it was a function.

          Comments
          1. By Anonymous Coward (212.77.163.100) on

            > > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
            >
            > foo = MIN(x++, y--);
            > After this runs, x will be incremented twice and y will be decremented twice, instead of once if it was a function.

            s/\<and\>/or/

          2. By tedu (udet) on


            > > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
            > > #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
            > >
            > >
            > > These are dangerous and stupid? Why?
            > The above could have unexpected side-effects if you do something like this:
            >
            > foo = MIN(x++, y--);

            If you don't know all caps means macro or if you write code like this even if min is a function, this will be far from the only mistake in your code.

            Comments
            1. By Anonymous Coward (75.31.211.86) on

              >
              > > > #define MIN(x1, x2) ((x1) < (x2) ? (x1) : (x2))
              > > > #define MAX(x1, x2) ((x1) > (x2) ? (x1) : (x2))
              > > >
              > > >
              > > > These are dangerous and stupid? Why?
              > > The above could have unexpected side-effects if you do something like this:
              > >
              > > foo = MIN(x++, y--);
              >
              > If you don't know all caps means macro or if you write code like this even if min is a function, this will be far from the only mistake in your code.
              >
              True, but depending on everyone to keep this convention is a bad idea. There are some cases where lowercase letters are also used for a macro. For instance, some implementations of putc() are actually macros to call fputc() with stdout as the other argument. (Not in OpenBSD though, which implements it as a function and actually goes through steps to #undef any putc macro in the code (lib/libc/stdio/putc.c) :)) I've also seen code elsewhere containing a macro called min() instead of MIN(). Of course this is bad practice.

              My personal feeling: I use macros for conditional compilation and that's it.

      2. By Anonymous Coward (212.20.215.132) on

        > > In Part I, the author recommends using the following macro to check if malloc returns an error :
        >
        > No no no no no!
        >
        > Macros are an abomination and should have long ago been consigned to the bin of clever but actually really dangerous and stupid ideas.
        >
        > I can't take seriously any article that suggests using macros. What are the authors thinking?

        Great. Next you'll probably say "No! Never use goto!" There's a time and place for both.

        Comments
        1. By Anonymous Coward (195.212.199.56) Rich on

          > Great. Next you'll probably say "No! Never use goto!" There's a time and place for both.
          >

          No there isn't. Unless you want to be REALLY sloppy with your code.

  3. By Rich (195.212.199.56) on

    A bit off topic, but as it's mentioned in the article...

    Does anyone know why chroot() can only be executed by root?

    It seems an unnecessary restriction (I can't spot the security issue, if there is one), and one that has forced me on more than one occasion to start applications as root and later drop privilege. If chroot() worked from non-root then I could have just started the app as a low privilege user in the first place, which would have been more secure because it would never have to be run as root, even if it is just for a very short time.

    Comments
    1. By Otto Moerbeek (otto) on http://www.drijf.net

      If ordinary users can chroot, they can easily fool programs like su(8) by providing the neccesary /etc/xyz files in the chroot dir.

      Comments
      1. By Rich (195.212.199.56) on

        > If ordinary users can chroot, they can easily fool programs like su(8) by providing the neccesary /etc/xyz files in the chroot dir.

        Oooo... good one.

        Is that the only issue? And doesn't this represent an issue with su rather than a reason to restrict use of chroot() ? I don't know how su works internally, but I would expect it to be immune to this sort of thing by being well and truly tied to the real /etc/ (ie - it would use the kernel's idea of what /etc/ is rather than the chrooted user's).

        A good point though. Thanks.

        Comments
        1. By Otto Moerbeek (otto) on http://www.drijf.net

          > Is that the only issue? And doesn't this represent an issue with su rather than a reason to restrict use of chroot() ? I don't know how su works internally, but I would expect it to be immune to this sort of thing by being well and truly tied to the real /etc/ (ie - it would use the kernel's idea of what /etc/ is rather than the chrooted user's).

          Actually, the kernel has no idea of those things, it has no concept of some files or dirs being different or special as compared to others.


      2. By Rich (195.212.199.56) on

        If what you say is true (and I'm not saying it is or it isn't - I don't know) then doesn't this also render (for example) useless/dangerous the recent addition to ssh (ie - chrooting logged in users)?

        Assuming you want to allow your logged-in user to create and use files in their chrooted environment then all they have to do is create their own /etc/ and exploit it in just the way you describe with potentially disastrous consequences.

        Comments
        1. By Otto Moerbeek (otto) on http://www.drijf.net

          > If what you say is true (and I'm not saying it is or it isn't - I don't know) then doesn't this also render (for example) useless/dangerous the recent addition to ssh (ie - chrooting logged in users)?
          >
          > Assuming you want to allow your logged-in user to create and use files in their chrooted environment then all they have to do is create their own /etc/ and exploit it in just the way you describe with potentially disastrous consequences.

          sshd enforces certain restrictions on the chroot direcory. See sshd_config(8) and http://undeadly.org/cgi?action=article&sid=20080220110039

        2. By Arach (87.103.146.206) on

          > Assuming you want to allow your logged-in user to create and use files in their chrooted environment then all they have to do is create their own /etc/ and exploit it in just the way you describe with potentially disastrous consequences.

          I this case, logged in users will be chrooted not to randomly chosen location, but exactly to where the root wants.

        3. By Rich (195.212.199.56) on

          Yes, of course. Scrub that - it was a silly thought.

        4. By tedu (67.111.202.10) on

          > If what you say is true (and I'm not saying it is or it isn't - I don't know) then doesn't this also render (for example) useless/dangerous the recent addition to ssh (ie - chrooting logged in users)?
          >
          > Assuming you want to allow your logged-in user to create and use files in their chrooted environment then all they have to do is create their own /etc/ and exploit it in just the way you describe with potentially disastrous consequences.

          This is why you shouldn't put su in the users chroot then.

  4. By Marc Espie (193.6.223.119) espie@openbsd.org on

    Just read the articles.

    So the author refers to OpenBSD as doing things right, big deal.
    I wouldn't trust him to write secure code. Not much attention to details, not a lot of depth to what he says.

    Examples:
    - numerous people have already commented on his use of macro. He also apparently doesn't understand exceptions. His article hints at an important point, without ever saying it explicitly: code that hasn't been run doesn't work. Which is why error recovery code is so often wrong. This has nothing to do with exceptions handling being analogous to gotos (it's not. there are pitfalls in exception handling, but they come from other issues entirely), and it has everything to do with code being bogus until you have actually run it: hence error code for exceptional situations should be kept as simple as possible, and run.
    - he correctly points out that pointers and ints are different beasts. He doesn't say how weird it is to store pointers in integer variables: don't worry about what size you should use, JUST DON'T DO IT. Most of the cases in existence are plain abuse.

    It also looks like he has problems with memory handling... he wants to wrap things in MORE complex code to make it work... good luck !

    One thing he gets is that complex code never works. One reason OpenBSD manages to remove some security issues from its base is because we follow the KISS principle. It's already tough enough writing correct simple code. So complex code ? that's beyond our skills...

Latest Articles

Credits

Copyright © - Daniel Hartmeier. All rights reserved. Articles and comments are copyright their respective authors, submission implies license to publish on this web site. Contents of the archive prior to as well as images and HTML templates were copied from the fabulous original deadly.org with Jose's and Jim's kind permission. This journal runs as CGI with httpd(8) on OpenBSD, the source code is BSD licensed. undeadly \Un*dead"ly\, a. Not subject to death; immortal. [Obs.]