Fix a segmentation fault if the private key file is corrupted

[lpefferkorn]

When the private key file is corrupted, cf-promises and cf-agent generate a segmentation fault:
Without the patch:

$ cf-promises 
2014-01-29T07:29:57+0100    error: Error reading private key. (PEM_read_RSAPrivateKey: too long)
Segmentation fault

With the patch:

$ cf-promises 
2014-01-29T07:25:22+0100    error: Error reading private key. (PEM_read_RSAPrivateKey: too long)
2014-01-29T07:25:22+0100    error: Fatal CFEngine error: Couldn't read private key from file /home/loic/.cfagent/ppkeys/localhost.priv

Feedback is welcome :-)

[ediosyncratic]

Looks sensible. I was about to ask you to free(secretkeyfile); but then I realised that FatalError never returns !

[jimis]

cf-agent is supposed to work even without keys (even though a warning is logged). This breaks all acceptance tests, @lpefferkorn please run "make check" before every pull request.

[vohi]

Sorry, my bad for merging. I revert that change.

@lpefferkorn could you provide a stack trace of the crash? A null-pointer check seems to be missing, just somewhere else (and not handled via FatalError).

[lpefferkorn]

@jimis I usually always run make check before submitting a PR, but I've made something wrong this time, sorry for the noise !

@vohi: The return value of LoadSecretKeys() is never checked, however PRIVKEY and PUBKEY are checked later, but not in this case. The stacktrace is self-explanatory (rev 9865d89).

I've not found any check of return value for this function, even if it's defined to return a bool. Is it expected ?

Starting program: /usr/local/sbin/cf-promises 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2014-01-29T19:56:04+0100    error: Error reading private key. (PEM_read_RSAPrivateKey: too long)

Program received signal SIGSEGV, Segmentation fault.
HashPubKey (key=0x0, digest=digest@entry=0x7fffffff77d0 "", type=HASH_METHOD_MD5) at files_hashes.c:131
131     if (key->n)
(gdb) bt
#0  HashPubKey (key=0x0, digest=digest@entry=0x7fffffff77d0 "", type=HASH_METHOD_MD5) at files_hashes.c:131
#1  0x00007ffff7b2a80e in PolicyHubUpdateKeys  (policy_server=policy_server@entry=0x61e220 "192.168.2.110") at crypto.c:197
#2  0x00007ffff7b46a7e in GenericAgentInitialize (ctx=ctx@entry=0x605010, config=config@entry=0x6059a0) at generic_agent.c:903
#3  0x00007ffff7b4708a in GenericAgentDiscoverContext (ctx=ctx@entry=0x605010,   config=config@entry=0x6059a0) at generic_agent.c:113
#4  0x000000000040244e in main (argc=1, argv=0x7fffffffec78) at cf-promises.c:125
(gdb) 

Regarding FatalError() use, it's up to you to decide if a corrupted private key should immediately exit or not :)

[cduclos]

I assume this happens on 3.5.x? Or does this happen in matter too?
29. jan. 2014 20:16 skrev "Loic Pefferkorn" notifications@github.com
følgende:

@jimis https://github.com/jimis I usually always run make check before
submitting a PR, but I've made something wrong this time, sorry for the
noise !

@vohi https://github.com/vohi: The return value of LoadSecretKeys() is
never checked, however PRIVKEY and PUBKEY are checked later, but not in
this case. The stacktrace is self-explanatory (rev 9865d899865d89).

I've not found any check of return value for this function, even if it's
defined to return a bool. Is it expected ?

Starting program: /usr/local/sbin/cf-promises
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
2014-01-29T19:56:04+0100 error: Error reading private key. (PEM_read_RSAPrivateKey: too long)

Program received signal SIGSEGV, Segmentation fault.
HashPubKey (key=0x0, digest=digest@entry=0x7fffffff77d0 "", type=HASH_METHOD_MD5) at files_hashes.c:131
131 if (key->n)
(gdb) bt
#0 HashPubKey (key=0x0, digest=digest@entry=0x7fffffff77d0 "", type=HASH_METHOD_MD5) at files_hashes.c:131
#1 0x00007ffff7b2a80e in PolicyHubUpdateKeys (policy_server=policy_server@entry=0x61e220 "192.168.2.110") at crypto.c:197
#2 0x00007ffff7b46a7e in GenericAgentInitialize (ctx=ctx@entry=0x605010, config=config@entry=0x6059a0) at generic_agent.c:903
#3 0x00007ffff7b4708a in GenericAgentDiscoverContext (ctx=ctx@entry=0x605010, config=config@entry=0x6059a0) at generic_agent.c:113
#4 0x000000000040244e in main (argc=1, argv=0x7fffffffec78) at cf-promises.c:125
(gdb)

Regarding FatalError() use, it's up to you to decide if a corrupted
private key should immediately exit or not :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/1358#issuecomment-33619187
.

[lpefferkorn]

@cduclos Segfault with current master (c6248f6), but ok with 3.5.3 (however the message can be improved a bit for the end-user?)

3.5.3:

 # cf-promises 
2014-01-29T21:50:33+0100    error: Error reading private key. (PEM_read_RSAPrivateKey: too long)

current master:

# cf-promises 
2014-01-29T21:53:20+0100    error: Error reading private key. (PEM_read_RSAPrivateKey: too long)
Segmentation fault