6429 SMB domain join doesn't work with libreSSL

Review Request #112 — Created Nov. 3, 2015 and submitted

Information
andy_js
illumos-gate
master
6429, 9546, 9547
Reviewers
general
citrus, gwr, vgusev, yuripv 
smbd segfaults in kerberos when attempting join a domain with libreSSL as the SSL implementation:


fca38752 OBJ_add_object (81e2f68, 814f918, 6, fcb08932, fcb08940, 81e2f68) + 22

fca39503 OBJ_create (fcb08924, fcb08932, fcb08940, fcaf8c3b, fcb1a484, 0) + e3

fcaf8cb0 pkinit_init_pkinit_oids (819a0c8, 0, 34, fcb03f6c, 1, 18) + 6b

fcafcd8f pkinit_init_plg_crypto (813056c, 0, 10, feb06db9, f8, 0) + 52

fcaf7965 pkinit_client_plugin_init (8090048, fd7d9a0c, f0, 80900f8, fd7d9a0c, 1) + 92

fccd58be krb5_init_preauth_context (8090048, 8130598, fccf663d, fcd0a000, fcd0a000, 8090048) + 1e0

fccd5c70 krb5_preauth_request_context_init (8090048, fd7d9b74, f, 8130598, 6, fccf663d) + 29

fccc9c9f krb5_get_init_creds (8090048, fd7daa28, 812fdf8, 0, 0, 0) + 42d

fccd33c0 __krb5_get_init_creds_password (8090048, fd7daa28, 812fdf8, fd7db5cd, 0, 0) + 14c

fccd38a6 krb5_get_init_creds_password (8090048, fd7daa28, 812fdf8, fd7db5cd, 0, 0) + 5e

fec5d947 smb_kinit (fd7db054, fd7db5a4, fd7db5cd, fe58b000, 8090048, fe6c0059) + 1b8

fec58086 smb_ads_open_main (fd7dadd0, fd7db054, fd7db5a4, fd7db5cd, 0, 4) + 42

fec588e0 smb_ads_join (fd7db054, fd7db5a4, fd7db5cd, fd7db29a, 0, 0) + a4

fe6d1448 mlsvc_join (fd7db4a4, fd7db39c, fd7db4a4, fd7db4a4, fd7db4a4, fd7db6b4) + 22f

0805a9e4 smbd_join_domain (fd7db4a4, fd7db39c, 0, fd7db4a4, fd7db6b4, 0) + 33

0805ab48 smbd_join (fd7db4a4, fd7db39c, 8056e60, fd7db4a4, fece0018, fece08e8) + 39

080596b7 smbd_dop_join (fd7db6b4, fed281ec, 12, fed281f7, 24, 24) + 7d

08059e74 smbd_door_dispatch_op (fd7db6b4, fd7db734, 24, 0, 0, 0) + 5c

0805a3d5 smbd_door_dispatch (80718c0, fd7db734, 6cc, 0, 0, 805a22f) + 1a6

feec6c2b __door_return () + 4b


The issue was fixed by backporting these changes to MIT Kerberos:

https://github.com/krb5/krb5/commit/8ee1790ba6e3468d7ed53ed46123dc9545a4216f

https://github.com/krb5/krb5/commit/6b9e570a7e98470b806a26c5119e53b2145e2586
With the patch applied smbd no longer segfaults when attempting to join a domain.


This fix has been in production for over two years without issue.
andy_js
andy_js
gwr
gwr 
  1. Could you please add some details to the bug report about the segfault?
    
i.e. stack backtrace and analysis?  Thx!
    1. Also, do you know if this works with openssl 0.9.8 and/or openssl 1.0?
    2. The issue was caused by a NULL pointer dereference in OBJ_create.  I'll see what I can do about getting my hands on a backtrace.
    3. How do we know whether this works with the openssl that OI and others use?
      
(openssl 0.9.8 and/or openssl 1.0, I think)
    4. The original changeset was meant for OpenSSL so I would expect it to work, but as far as I remember I haven't tested it with OpenSSL.
    5. OK, but I guess that's the hold-up on getting this integrated.  Someone needs to test it.
      
Sorry, I don't have time right now...
    6. FYI, I tested this change on a recent OI system.  AD join worked fine.
  2. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 





 

  

   




andy_js




   

   





  

 



 

  


   

   

    
 
andy_js


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I think it's worth noting that the (admittedly rather trivial) changes required for LibreSSL to build on illumos have now been upstreamed.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




andy_js




   

   





  

 



 

  


   

   

    
 
andy_js


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This change has been in production for over a year now.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




andy_js




   

   

  

 



 















 





 

  

   




yuripv




   

   







  

 



 

  


   

   

    
 
yuripv


    
   


  

  

  



    




  1. 
 
    
  
    
   
    LGTM if it doesn't break the builds using OpenSSL.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




andy_js




   

   

  

 



 















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    As I understand second commit from github just removes allocation OBJ_create where segfault occurs. I guess origin ticket is
    

    http://krbdev.mit.edu/rt/Ticket/Display.html?id=7889
    

    But neigher github commits nor ticket 7889 don't have mention to 'segfault' or something similar and just have word "not thread-safe".
    
So my opinion these commits could be fixing commits. Thanks for stack trace, it helps to understand issue and fixes even in years.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 



 

  

   




andy_js




   

   

  

 



 















 





 

  

   




gwr




   

   





  

 



 

  


   

   

    
 
gwr


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Have you tested your latest code with all of: libreSSL, OpenSSL 1.0, OpenSSL 1.1
    
That will probably be required before integration.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      No, but the pre-8982 version of the changes were tested on OmniOS (OpenSSL 1.0) and there were no issues.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      We're also running the pre-8982 version of the changes in production with LibreSSL since 3 years.
      
 
      

      3. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




citrus




   

   







  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tested this patch against OmniOS bloody building with OpenSSL 1.1 and with pkinit pre-auth to a local KDC. Worked fine before and after patch, so LGTM.
    

    

    May 29 13:02:33 bloody krb5kdc[312](info): AS_REQ (7 etypes {18 17 16 23 24 3 1}) 172.27.10.9: NEEDED_PREAUTH: bloody.omniosce.org@OMNIOSCE.ORG for krbtgt/OMNIOSCE.ORG@OMNIOSCE.ORG, Additional pre-authentication required
May 29 13:02:36 bloody krb5kdc[312](info): AS_REQ (7 etypes {18 17 16 23 24 3 1}) 172.27.10.9: ISSUE: authtime 1527598956, etypes {rep=18 tkt=18 ses=18}, bloody.omniosce.org@OMNIOSCE.ORG for krbtgt/OMNIOSCE.ORG@OMNIOSCE.ORG
bloody# klist
Ticket cache: FILE:/tmp/krb5cc_0
Default principal: bloody.omniosce.org@OMNIOSCE.ORG

Valid starting                Expires                Service principal
05/29/18 13:02:36  05/29/18 21:02:36  krbtgt/OMNIOSCE.ORG@OMNIOSCE.ORG
        renew until 06/05/18 13:02:36

    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Thanks!  That's pretty much what I expected.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This patch passed internal review some months ago.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




kmays




   

   







  

 



 

  


   

   

    
 
kmays


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




andy_js




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...