Bug 49383 - Several univention-python-heimdal bugs - SIGSEGV, memory corruption
Several univention-python-heimdal bugs - SIGSEGV, memory corruption
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: Kerberos
UCS 4.4
Other Linux
: P5 normal (vote)
: UCS 4.4-3-errata
Assigned To: Philipp Hahn
Arvid Requate
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-29 14:05 CEST by Philipp Hahn
Modified: 2020-03-11 14:41 CET (History)
2 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 7: Crash: Bug causes crash or data loss
Who will be affected by this bug?: 1: Will affect a very few installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.080
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:
hahn: Patch_Available+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Philipp Hahn univentionstaff 2019-04-29 14:05:13 CEST
While migrating univention-python-heimdal to dh_python2 (Bug #49139) I found several bugs leading to crashes:

1. Dereference pointer instead of comparing it with an uninitialized integer:
> keyblock.c:118:24: warning: comparison between pointer and integer
>    enctype = enctype_obj>enctype;
>                         ^
> keyblock.c:118:24: warning: ‘enctype’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    enctype = enctype_obj>enctype;
>              ~~~~~~~~~~~^~~~~~~~

PATCH: sed -i -e '118s/>/->/' keyblock.c


2. Missing error handling:
> creds.c:127:3: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result [-Wunused-result]
>    asprintf(&s, "unknown (%d)", t.enc_part.etype);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> keytab.c:207:6: warning: variable ‘error’ set but not used [-Wunused-but-set-variable]
>   int error = 0;
>       ^~~~~
> keytab.c:235:4: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result [-Wunused-result]
>     asprintf(&etype, "unknown (%d)", entry.keyblock.keytype);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


3. Wrong type:
> context.c:87:32: warning: comparison between ‘krb5_enctype {aka enum ENCTYPE}’ and ‘enum <anonymous>’ [-Wenum-compare]
>   for (i=0; etypes && etypes[i] != ETYPE_NULL; i++) {
>                                 ^~

PATCH: sed -i -e '87:s/ETYPE_NULL/KRB5_ENCTYPE_NULL/' context.c


4. keytab.add() clobbers immutable string:
> keytab.c:181 »···»···memset (password_string, 0, strlen(password_string));

python -c 'import heimdal;P="univention";heimdal.keytab(heimdal.context(), "").add("a@B.C", 0, "des-cbc-md5", P, 0, 0);print(P)'

Why is this bad:
>>> a = "univention"
>>> b = "univention"
>>> hash(a), hash(b)
(6976887264700423245, 6976887264700423245)
>>> import heimdal as h;h.keytab(h.context(), "").add("a@B.C", 0, "des-cbc-md5", a, 0, 0);
>>> a
'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>> b
'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
Python internally hashed any string and only ever stores one instance of the text. If the password passed to keytab.add() by pure change is equal to any other string instance (in the process), the string their is clobbered as well.

PATCH: sed -i -e '181d' keytab.c


5. dir(keytab) crashed with SIGSEGV:
python -c 'import heimdal;dir(heimdal.keytab(heimdal.context(), ""))'

Program received signal SIGSEGV, Segmentation fault.
Py_FindMethodInChain (chain=0x7fffffffe200, self=<Keytab at remote 0x7ffff7f790b0>, name=0x55555576aee6 "__dict__")
    at ../Objects/methodobject.c:370
370     ../Objects/methodobject.c: Datei oder Verzeichnis nicht gefunden.
(gdb) bt
#0  Py_FindMethodInChain (chain=0x7fffffffe200, self=<Keytab at remote 0x7ffff7f790b0>, name=0x55555576aee6 "__dict__")
    at ../Objects/methodobject.c:370
#1  0x0000555555725419 in Py_FindMethod (methods=<optimized out>, self=<optimized out>, name=<optimized out>)
    at ../Objects/methodobject.c:389
#2  0x0000555555669e3d in _generic_dir (obj=<Keytab at remote 0x7ffff7f790b0>) at ../Objects/object.c:1863
#3  _dir_object (obj=<Keytab at remote 0x7ffff7f790b0>) at ../Objects/object.c:1940
#4  PyObject_Dir () at ../Objects/object.c:1977
#5  0x0000555555669946 in builtin_dir.lto_priv () at ../Python/bltinmodule.c:598
#6  0x000055555564f84a in call_function (oparg=<optimized out>, pp_stack=0x7fffffffe338) at ../Python/ceval.c:4352
#7  PyEval_EvalFrameEx () at ../Python/ceval.c:2989
#8  0x000055555564d9f5 in PyEval_EvalCodeEx () at ../Python/ceval.c:3584
#9  0x000055555564d7b9 in PyEval_EvalCode (co=<optimized out>, globals=<optimized out>, locals=<optimized out>)
    at ../Python/ceval.c:669
#10 0x00005555556ad3b6 in run_mod.lto_priv.2003 (arena=0x555555bc9b20, flags=0x7fffffffe52c, 
    locals={'__builtins__': <module at remote 0x7ffff7fa4b08>, '__name__': '__main__', 'heimdal': <module at remote 0x7ffff7f814e8>, '__doc__': None, '__package__': None}, 
    globals={'__builtins__': <module at remote 0x7ffff7fa4b08>, '__name__': '__main__', 'heimdal': <module at remote 0x7ffff7f814e8>, '__doc__': None, '__package__': None}, filename=0x5555557768eb "<string>", mod=<optimized out>) at ../Python/pythonrun.c:1376
#11 PyRun_StringFlags () at ../Python/pythonrun.c:1339
#12 0x00005555556ae07c in PyRun_SimpleStringFlags () at ../Python/pythonrun.c:974
#13 0x00005555556294a9 in Py_Main () at ../Modules/main.c:584
#14 0x00007ffff6f182e1 in __libc_start_main (main=0x5555556290a0 <main>, argc=3, argv=0x7fffffffe6e8, init=<optimized out>, 
    fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe6d8) at ../csu/libc-start.c:291
#15 0x0000555555628f9a in _start ()




6. Exceptions are named all wrong:
python -c 'import heimdal;print(dir(heimdal))'

They are all named similar to '(-1765328379L)'; this is due the nested use of C pre-processor macros: 

PATCH: sed -i '72{h;d};80{p;g;s/\<o\>/e/}' error.c


7. Many more issues already fixed in my WIP git branch: phahn/49139_dhpy2-heimdal
Comment 1 Philipp Hahn univentionstaff 2019-05-02 08:42:51 CEST
8. Missing type checks:
  python -c 'import heimdal as h;h.principal("", "")' -> SIGSEGV

 PyArg_ParseTuple(args, "Os", &context, &enc)
is used which accepts any Python object, which is then cast to "krb5_context" without checking if "context" is of the correct type.
Comment 2 Philipp Hahn univentionstaff 2019-12-08 23:39:39 CET
git:phahn/49139_dhpy2-heimdal
Comment 3 Philipp Hahn univentionstaff 2019-12-18 17:53:25 CET
[4.4-3] bb118a0e46 Bug #50475,#49383,#38736 Heimdal: Merge branch 'phahn/49139_dhpy2-heimdal' into 4.4-3

Package: univention-python-heimdal
Version: 9.0.0-3A~4.4.0.201912181749
Branch: ucs_4.4-0
Scope: errata4.4-3

[4.4-3] 6740211353 Bug #50475: univention-python-heimdal 9.0.0-3A~4.4.0.201912181749
 doc/errata/staging/univention-python-heimdal.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 4 Philipp Hahn univentionstaff 2019-12-18 18:39:12 CET
QA: There is a unit test "test.py", which you even can execute on an old system. Most tests will crash due to bugs in the old heimdal.so. You have to execute them one-by-one manually like this:

python2 test.py TestRefcount.test_context
python2 test.py TestRefcount.test_principal
python2 test.py TestRefcount.test_creds
python2 test.py TestRefcount.test_keytab
python2 test.py TestRefcount.test_salt
python2 test.py TestRefcount.test_enctype
python2 test.py TestRefcount.test_keyblock
python2 test.py TestRefcount.test_exception
python2 test.py TestContext.test_get_permitted_enctypes
python2 test.py TestContext.test_dir
python2 test.py TestPrincipal.test_type  # FIXED
python2 test.py TestPrincipal.test_principal
python2 test.py TestPrincipal.test_realm  # NEW
python2 test.py TestPrincipal.test_dir
python2 test.py TestCreds.test_parse
python2 test.py TestCreds.test_change_password
python2 test.py TestCreds.test_dir  # CRASHED
python2 test.py TestKeytab.test_keytab_missing
python2 test.py TestKeytab.test_keytab_empty
python2 test.py TestKeytab.test_keytab_add
python2 test.py TestKeytab.test_keytab_remove_missing
python2 test.py TestKeytab.test_keytab_remove_existing
python2 test.py TestKeytab.test_dir
python2 test.py TestSalt.test_salt
python2 test.py TestSalt.test_salt_raw
python2 test.py TestSalt.test_dir
python2 test.py TestEnctype.test_type  # FIXED
python2 test.py TestEnctype.test_enctype
python2 test.py TestEnctype.test_dir
python2 test.py TestKeyblock.test_keyblock_principal
python2 test.py TestKeyblock.test_keyblock_salt
python2 test.py TestKeyblock.test_keyblock_raw
python2 test.py TestKeyblock.test_dir
python2 test.py TestCcache.test_list
python2 test.py TestCcache.test_init  # CRASHED
python2 test.py TestCcache.test_use_after_destroy  # CRASHED
python2 test.py TestCcache.test_store_cred
python2 test.py TestASN1.test_asn1_decode_key
python2 test.py TestASN1.test_asn1_decode_key_with_context  # NEW
python2 test.py TestASN1.test_asn1_encode_key
python2 test.py TestException.test_KRB5KDC_ERR_NONE  # FIXED
python2 test.py TestException.test_exception  # FIXED
Comment 5 Philipp Hahn univentionstaff 2019-12-19 10:37:44 CET
This breaks <https://jenkins.knut.univention.de:8181/job/UCS-4.4/job/UCS-4.4-3/job/S4Connector/20/>
52_s4connector.030_sync_with_activated_pwqualitycheck.master071c
52_s4connector.109sync_create_and_modify_ucs_to_ad.master071c
52_s4connector.401check_posix_pwd_expiry_after_ad_pwdchange.master071c
52_s4connector.030_sync_with_activated_pwqualitycheck.master091c
52_s4connector.401check_posix_pwd_expiry_after_ad_pwdchange.master091c
Comment 6 Philipp Hahn univentionstaff 2019-12-19 12:31:23 CET
For now I removed the binary packages form ucs_4.4-0-errata4.4-3:

Possible cause:

Terminating running univention-cli-server processes.
Traceback (most recent call last):
  File "/usr/share/univention-s4-connector/sync_krbtgt", line 190, in <module>
    main()
  File "/usr/share/univention-s4-connector/sync_krbtgt", line 184, in main
    s4.sync_password()
  File "/usr/share/univention-s4-connector/sync_krbtgt", line 159, in sync_password
    krb5Key_new = univention.s4connector.s4.password.calculate_krb5key(unicodePwd_attr, supplementalCredentials, int(msDS_KeyVersionNumber))
  File "/usr/lib/python2.7/dist-packages/univention/s4connector/s4/password.py", line 72, in calculate_krb5key
    keys.append(heimdal.asn1_encode_key(key, None, kvno))
TypeError: argument 2 must be heimdal.krb5Salt, not None
Comment 7 Philipp Hahn univentionstaff 2020-01-15 10:33:16 CET
[4.4-3] d429b13426 Bug #50475: univention-python-heimdal 9.0.0-3A~4.4.0.202001151023
 doc/errata/staging/univention-python-heimdal.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[4.4-3] 2cd5de4baf Bug #49383 heimdal: Replace magic KRB5_ENCTYPEs
 base/univention-python-heimdal/module.c | 52 +++++++++++++++++++++++++++++
 base/univention-python-heimdal/test.py  | 58 ++++++++++++++++-----------------
 2 files changed, 81 insertions(+), 29 deletions(-)

[4.4-3] 6271592ae7 Bug #49383 heimdal: Initialize variables
 base/univention-python-heimdal/asn1.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

[4.4-3] 78c4992e35 Bug #49383 heimdal: Fix asn1_encode_key(..., None, ...)
 base/univention-python-heimdal/asn1.c  | 11 +++++++----
 base/univention-python-heimdal/test.py | 12 ++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

Package: univention-python-heimdal
Version: 9.0.0-3A~4.4.0.202001151023
Branch: ucs_4.4-0
Scope: errata4.4-3

FYI: There are still 5 more cleanup patches for S4C sync_krbtgt in my <git:phahn/49139_dhpy2-heimdal> which I created by debugging the last regression.
Comment 8 Arvid Requate univentionstaff 2020-01-28 10:08:44 CET
* Fix reference counting / object ownership for some dict and list elements
* Initialize context in asn1_decode_key
* Add TestRefcount
* Several additions of PyErr_NoMemory
* Fix ccache_destroy() and test with TestCcache
* Make PyArg_ParseTuple check python method agument types && test with test_type
* Initialize structures properly with {0}/NULL/memset(0)
* Rename *_destroy to *_dealloc
  (context, creds, enctype, keyblock, realm, ticket)
* Use krb5ContextObject instead of raw krb5_context to implement proper
  reference counting.
* Include krb5_context and krb5_keytab structures directly in
  krb5KeytabObject structure and let Python handle the memory allocation.
* Disable unused functions (realm_from_realm, salt_from_salt)
* Explicitly declare methods without arguments as METH_NOARGS
* Declare functions as static that are only required internally
* Move static variables from header files to c files
* Terminate method arrays by {NULL} element
* Whitespace cleanup and adjust to "new" german spelling
* Don't memset immutable Python strings
* Fix code typos (broken enctype_obj>enctype instead of enctype_obj->enctype)
* Extend unit test
Comment 9 Erik Damrose univentionstaff 2020-03-11 14:41:44 CET
<http://errata.software-univention.de/ucs/4.4/477.html>