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.

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

NULL Pointer Dereference vulneribility in igraph_i_strdiff function #1141

Closed
leonzhao7 opened this issue Dec 4, 2018 · 4 comments

Comments

@leonzhao7
Copy link

commented Dec 4, 2018

Test Version

dev version, git clone https://github.com/igraph/igraph

Test Program

modify graphml.c in examples/simple directory

#include <igraph.h>
#include <stdio.h>
#include <unistd.h>             /* unlink */

void custom_warning_handler (const char *reason, const char *file,
                             int line, int igraph_errno) {
  printf("Warning: %s\n", reason);
}

int main(int argc, char **argv) {
  igraph_t g;
  igraph_error_handler_t* oldhandler;
  igraph_warning_handler_t* oldwarnhandler;
  int result;
  FILE *ifile;

  if (argc < 2) {
    printf("Error.\n");
    return 0;
  }

  /* GraphML */
  ifile=fopen(argv[1], "r");
  if (ifile==0) {
    return 10;
  }

  oldhandler=igraph_set_error_handler(igraph_error_handler_ignore);
  oldwarnhandler=igraph_set_warning_handler(custom_warning_handler);
  if ((result=igraph_read_graph_graphml(&g, ifile, 0))) {
    // maybe it is simply disabled at compile-time
    if (result == IGRAPH_UNIMPLEMENTED) return 77;
    return 1;
  }
  igraph_set_error_handler(oldhandler);

  fclose(ifile);

  printf("The directed graph:\n");
  printf("Vertices: %li\n", (long int) igraph_vcount(&g));
  printf("Edges: %li\n", (long int) igraph_ecount(&g));
  printf("Directed: %i\n", (int) igraph_is_directed(&g));
  igraph_write_graph_edgelist(&g, stdout);
  igraph_destroy(&g);

  return 0;
}

and gcc graphml.c -ligraph -o graphml
graphml [infile]

Gdb and Backtrace

Reading symbols from graphml...(no debugging symbols found)...done.
(gdb) run igraph_trie-igraph_i_strdiff-112.crash
Starting program: /root/igraph/examples/simple/graphml igraph_trie-igraph_i_strdiff-112.crash

Program received signal SIGSEGV, Segmentation fault.
igraph_i_strdiff (str=0x61a7f0 "d", key=key@entry=0x0) at igraph_trie.c:112
112       while (key[diff] != '\0' && str[diff] != '\0' && str[diff]==key[diff]) {

p key
$2 = 0x0

(gdb) l
107      */
108
109     long int igraph_i_strdiff(const char *str, const char *key) {
110
111       long int diff=0;
112       while (key[diff] != '\0' && str[diff] != '\0' && str[diff]==key[diff]) {
113         diff++;
114       }
115       return diff;
116     }

(gdb) bt
#0  igraph_i_strdiff (str=0x61a7f0 "d", key=key@entry=0x0) at igraph_trie.c:112
#1  0x00007ffff7869323 in igraph_trie_get_node (t=t@entry=0x7fffffffd180, key=key@entry=0x0, newvalue=newvalue@entry=-1, id=id@entry=0x7fffffffcd30)
    at igraph_trie.c:139
#2  0x00007ffff7869e4e in igraph_trie_check (t=t@entry=0x7fffffffd180, key=key@entry=0x0, id=id@entry=0x7fffffffcd30) at igraph_trie.c:361
#3  0x00007ffff78a291c in igraph_i_graphml_attribute_data_finish (state=0x7fffffffcfa0) at foreign-graphml.c:810
#4  0x00007ffff78a2dd5 in igraph_i_graphml_sax_handler_end_element_ns (state0=0x7fffffffcfa0, localname=<optimized out>, prefix=<optimized out>,
    uri=<optimized out>) at foreign-graphml.c:1172
#5  0x00007ffff7037343 in ?? () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#6  0x00007ffff703fc05 in ?? () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#7  0x00007ffff704177b in xmlParseChunk () from /usr/lib/x86_64-linux-gnu/libxml2.so.2
#8  0x00007ffff78a3129 in igraph_read_graph_graphml (graph=<optimized out>, instream=0x614c20, index=<optimized out>) at foreign-graphml.c:1368
#9  0x0000000000400c0f in main ()

POC file

igraph_trie-igraph_i_strdiff-112.zip

CREDIT

Zhao Liang, Huawei Weiran Labs

@ntamas

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

The input is not valid XML, although it probably shouldn't crash igraph anyway. I'll take a look at this.

@ntamas ntamas self-assigned this Dec 4, 2018
@ntamas ntamas closed this in e3a9566 Dec 6, 2018
@tillea

This comment has been minimized.

Copy link

commented Dec 25, 2018

I've applied this patch to the Debian package. Recently I was asking for a new release (issue #1107 ). I really think you should do a release containing fixes for CVEs. Otherwise you risk that user who rely on releases are running insecure software. Thank you, Andreas.

@szhorvat

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2018

I am absolutely not arguing against a release, just wanted to note that the importance of such security issues is way overblown. igraph is research software. It's not used on webservers or other security-critical applications. Bugs like this will make little practical difference.

The priority should be on producing correct results for reasonable inputs, so users can have confidence in their analysis results. Segfaults with bad inputs won't lead to flawed research papers. Incorrect result bugs will. If I were to cherry pick fixes, I would focus on such bugs, not security.

@tillea

This comment has been minimized.

Copy link

commented Dec 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.